Remove "final" keyword and make methods protected. #14517
Merged
Conversation
This file contains 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
This adds extensibility to the API and makes custom implementation of the server possible.
|
|
|
|
|
|
|
|
|
|
|
wcevans
reviewed
Feb 27, 2018
| g_core_codegen_interface->gpr_inf_future( | ||
| GPR_CLOCK_REALTIME)) != SHUTDOWN); | ||
| } | ||
| virtual bool Next(void** tag, bool* ok); |
There was a problem hiding this comment.
Do we really want to eat a virtual dispatch on CompletionQueue::Next()? This is part of every single gRPC user's datapath. If you want to allow multiple CompletionQueue implementations, consider allowing the user to avoid the virtual dispatch by providing a subclass that is final.
There was a problem hiding this comment.
Is there a benchmark showing that this is a problem? I'm not seeing it right now. If there is a benchmark, then that's a different story.
There was a problem hiding this comment.
I am a little concerned that CQ::Next() can no longer be inlined even for a minimal implementation of gRPC, but I suppose that FDO solves this problem for anyone who would care. I concede.
|
added 2 commits
March 9, 2018 09:39
Per style guide, the member variables are not protected but private and accessed through methods.
vjpai
suggested changes
Mar 9, 2018
There was a problem hiding this comment.
Please also get feedback from @dgquintas particularly wrt codegen header issues.
| @@ -175,6 +153,31 @@ class Server final : public ServerInterface, private GrpcLibraryCodegen { | |||
| /// \param num_cqs How many completion queues does \a cqs hold. | |||
| void Start(ServerCompletionQueue** cqs, size_t num_cqs) override; | |||
|
|
|||
| grpc_server* server() override { return server_; }; | |||
There was a problem hiding this comment.
Having this be protected is concerning, for the reason that this means that this is adding an API that exposes gRPC core structures. I'm busy in #14648 trying to remove any APIs that expose gRPC core structures (since they're technically not really API anyway). I thought your plan was to not use gRPC core structures in your derived classes anyway?
There was a problem hiding this comment.
The plan is to not use gRPC core structures unless it makes sense and avoids duplication. This is currently the only instance of using core (so we can reuse the channel creation, connection opening etc).
There was a problem hiding this comment.
Without having looked deeply into the details, using inheritance for code reuse is usually an anti-patter. Favor composition over inheritance for this.
There was a problem hiding this comment.
The issue was a desire to slip in a changed server in a variant library without requiring any application changes.
There was a problem hiding this comment.
Additionally, although this is moving the code from private to protected, it doesn't actually change anything because the base-class has it as protected. So this is not an issue.
include/grpcpp/server_builder.h
Outdated
| std::vector<std::reference_wrapper<NamedService>> service_refs; | ||
| for (auto &ptr : services_) { | ||
| service_refs.push_back(std::ref(*ptr)); | ||
| } |
There was a problem hiding this comment.
So this is a vector of references to unique_ptr? Given that you're not transferring ownership, why not just have this be a vector of dumb pointers acquired through ->get on each element of the iterator? I think that's more consistent with our overall style.
src/cpp/server/server_cc.cc
Outdated
| sync_cq_timeout_msec)); | ||
| if (sync_server_cqs_ != nullptr) { | ||
| for (auto it = sync_server_cqs_->begin(); it != sync_server_cqs_->end(); | ||
| it++) { |
There was a problem hiding this comment.
So we used to use the begin/end format all the time because of supporting pre-C++11 compilers, but we don't anymore, so can you change this to a range? I'm asking everyone to do this opportunistically any time they make a change in the C++ wrapping that affects one of these.
| #include <gtest/gtest.h> | ||
|
|
||
| namespace grpc { | ||
|
|
||
| // Unused implementation for the virtual "Next" method. | ||
| bool CompletionQueue::Next(void** tag, bool* ok) { return false; } |
There was a problem hiding this comment.
@dgquintas : does this feel kosher to you? We need a mechanism to allow the use of codegen headers without crapping out on linker errors now that we're using virtual members for some classes that get used in codegen. Can you comment on this?
There was a problem hiding this comment.
including codegen headers around is never a problem. The problem is putting non-codegen things into the codegen hierarchy. This should be fine AFAICT.
Please enter the commit message for your changes. Lines starting
|
|
|
|
|
|
Removed unnecessary "virtual" keyword from CompletionQueue::Next. Reverted two files that no longer need to be changed.
|
|
|
|
vjpai
suggested changes
Mar 13, 2018
| @@ -175,6 +153,31 @@ class Server final : public ServerInterface, private GrpcLibraryCodegen { | |||
| /// \param num_cqs How many completion queues does \a cqs hold. | |||
| void Start(ServerCompletionQueue** cqs, size_t num_cqs) override; | |||
|
|
|||
| grpc_server* server() override { return server_; }; | |||
There was a problem hiding this comment.
Additionally, although this is moving the code from private to protected, it doesn't actually change anything because the base-class has it as protected. So this is not an issue.
include/grpcpp/server_builder.h
Outdated
| private: | ||
| friend class ::grpc::testing::ServerBuilderPluginTest; | ||
|
|
||
| protected: | ||
| struct Port { |
There was a problem hiding this comment.
Let's also add experimental tags on these structs that have been moved from private to protected.
include/grpcpp/server_builder.h
Outdated
| struct Port { | ||
| grpc::string addr; | ||
| std::shared_ptr<ServerCredentials> creds; | ||
| int* selected_port; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<grpc::string> HostString; |
For "struct Port" and friends, since they are now protected instead of private.
|
|
vjpai
suggested changes
Mar 13, 2018
| private: | ||
| grpc_cq_polling_type polling_type_; | ||
| friend class ServerBuilder; | ||
| protected: |
There was a problem hiding this comment.
Ok, we have a core leakage again. Sorry for not catching this earlier. By promoting the ServerCompletionQueue constructor to protected, it has become API. However, this API has an argument that is a core enum. Can you look at your callsites and perhaps just elevate a constructor for a specific case to protected (since I assume that you have a subset of the possible polling type choices) and make that a bool argument or something that then delegates something else to the pre-existing private constructor?
include/grpcpp/server.h
Outdated
| std::mutex mu_; | ||
| // Server status |
There was a problem hiding this comment.
Since I'm requesting changes again, can I ask you to dedupe this comment?
|
vjpai
approved these changes
Mar 13, 2018
|
1 similar comment
|
|
Force merging this because CLA bot seems broken. |
|
|
|
|
Nico, I think the CLA bot was legitimately concerned. Makarand's commits were ill-formed because his config had a missing email address. |
|
|
|
|
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This adds extensibility to the API and makes custom implementation
of the server possible.