CompletionQueue DoThenAsyncNext#13084
Merged
kpayson64 merged 1 commit intogrpc:masterfrom Oct 25, 2017
Merged
Conversation
|
|
ctiller
reviewed
Oct 20, 2017
Member
There was a problem hiding this comment.
can we do:
template <typename T, typename F>
NextStatus DoThenAsyncNext(F&& f, /* ... rest of args ... */) {
// ... rest of code
}and avoid unnecessarily instantiating a std::function (since this is not for free, and lambda's pretty much are)
ctiller
reviewed
Oct 20, 2017
Member
There was a problem hiding this comment.
similarly here, and hoist the implementation into the header
ctiller
reviewed
Oct 20, 2017
Member
There was a problem hiding this comment.
Can we make a small class to handle the TLS cache here... constructor calls grpc_prepare... destructor does the cleanup (and could be in a cc file out of line since it's sufficiently complex)
ctiller
reviewed
Oct 20, 2017
Member
There was a problem hiding this comment.
Variable names should be lower case
ctiller
reviewed
Oct 20, 2017
Member
There was a problem hiding this comment.
== nullptr would be better here (for new code)
|
Contributor
Author
|
Addressed comments from ctiller's review. |
|
|
1 similar comment
|
ctiller
previously requested changes
Oct 23, 2017
Member
There was a problem hiding this comment.
F&&
(we want to be able to pass l- and r- values here)
Member
There was a problem hiding this comment.
Maybe make this a private member class of CompletionQueue (public names are forever)... If we need to we can promote later
test/cpp/qps/client_async.cc
Outdated
Member
There was a problem hiding this comment.
F&& change should allow this to be passed to DoThen directly, which would be preferable
Contributor
Author
|
Addressed feedback/added unit tests. |
|
|
|
|
vjpai
suggested changes
Oct 24, 2017
include/grpc/grpc.h
Outdated
Contributor
There was a problem hiding this comment.
grpc_cq_tls_cache_prepare to be more consistent with naming throughout the core API (object name first, then action verb). Additionally, since this is API, I would suggest not using "tls" because of abbreviation-ambiguity. (Note that C++ style guide doesn't like abbreviations.) Please call it grpc_cq_thread_local_cache or grpc_thread_local_cq or something like that
Contributor
There was a problem hiding this comment.
Additionally, even though these are experimental, please document the parameters and return values (more important for flush than prepare)
include/grpc/grpc.h
Outdated
Contributor
There was a problem hiding this comment.
Please add a comment above indicating what is the purpose of these TLS (implementation of cq thread-local cache, etc)
Contributor
There was a problem hiding this comment.
Since the cache implementation here is essentially a single element in a single CQ, these variable names are misleading. Can you call it g_cached_event and g_cached_cq or something like that? I would also add a comment saying that in the current implementation, only 1 event for 1 CQ is ever cached at a thread, to make it clear that this is the current cache characteristic. This is quite fine for the use case, but the name "cache" suggests the possibility of more than that (which is fine from an API perspectiv, since the current API does not preclude that in the unlikely event that it is ever desired in the future.)
Contributor
|
I've requested changes, but they're mostly cosmetic. |
Contributor
Author
|
Addressed comments |
|
|
|
|
vjpai
reviewed
Oct 24, 2017
grpc.def
Outdated
Contributor
There was a problem hiding this comment.
Please rename to grpc_thread_local_completion_queue_cache_init because it is more rhythmic (can be sung to the tune of Supercalifragilisticexpialidocious)
|
|
|
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an experimental function to C++ Completion Queues DoThenAsyncNext. It allows users to pass a lambda to be executed before the AsyncNext. If the lambda triggers a completion event, it is guaranteed to be delivered on the AsyncNext call.