[kudu-CR] [location awareness] Register client with location when connect to cluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/11923/7/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/11923/7/src/kudu/client/client.cc@667 PS7, Line 667: return data_->location(); Any particular reason not to hoist the KuduClient::Data::location() accessor into here a la GetHiveMetastore...? That is, do we actually need both accessors, or could we get by with just the KuduClient accessor? -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 7 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 18 Dec 2018 22:20:50 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Will Berkeley has uploaded a new patch set (#7) to the change originally created by Fengling Wang. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. [location_awareness] Register client with location when connect to cluster This patch enables the client to get a location assigned after connected to the cluster. After that, the client can compare its location with the replica locations to select which replica to read. Change-Id: I0efb327293d86168a30b05305f69d011ad15587a --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 10 files changed, 132 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/11923/7 -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 7 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Register client with location when connect to cluster
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/11923 )
Change subject: [location_awareness] Register client with location when connect
to cluster
..
Patch Set 6:
(8 comments)
This first new PS is just a test run... no need for review until I submit
another PS with an updated commit message.
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/client/client-internal.cc@714
PS6, Line 714: location_ = connect_response.client_location();
> Does location_ need to be protected by leader_master_lock_? Why or why not?
It needs to be protected by a lock- after a future patch, it may be used from
one thread to select replicas to scan while the client is reconnecting to the
cluster in another thread and being assigned a new location.
leader_master_lock_ seems appropriate to reuse because it protects other things
set when receiving a ConnectToMaster response from the leader master.
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master-test.cc@1626
PS6, Line 1626: TestConnectToMasterRegisterLocation
> It would be nice to have a scenario to verify how the system behaves in cas
Done
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master-test.cc@1635
PS6, Line 1635: ASSERT_OK(proxy_->ConnectToMaster(req, , ));
> Should also ASSERT that !resp.has_error.
Done
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master_service.cc@536
PS6, Line 536: resp->set_client_location(location);
> generic/paranoid nit: if GetClientLocation() didn't return Status::OK(), I
Done
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.h
File src/kudu/master/ts_descriptor.h:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.h@182
PS6, Line 182: class ClientDescriptor {
> Why is GetClientLocation stored in this class? Do you envision adding more
I dropped this class and exposed GetLocationFromLocationMappingCommand as a
function. This is a temporary state of affairs. A follow-up will factor that
out into some kind of LocationAssigner class, probably with some caching policy.
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.cc@410
PS6, Line 410: if (!location_mapping_cmd.empty()) {
> style code/readability nit: prefer the 'return early' style here, i.e.
N/A. Dropped this function in favor of (temporarily) exposing
GetLocationFromLocationMappingCommand.
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.cc@411
PS6, Line 411: string location_temp;
> I don't think this indirection is needed; GetLocationFromLocationMappingCmd
Ditto.
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.cc@415
PS6, Line 415: if (s.ok()) {
: *location = std::move(location_temp);
: } else {
: KLOG_EVERY_N_SECS(ERROR, 60) << Substitute(
: "Unable to assign location to client: $0",
: registration.rpc_addresses(0).host());
: return s;
: }
> the same style nit: prefer the 'return early' style
Ditto.
--
To view, visit http://gerrit.cloudera.org:8080/11923
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a
Gerrit-Change-Number: 11923
Gerrit-PatchSet: 6
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Fengling Wang
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 18 Dec 2018 19:05:45 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11923 )
Change subject: [location_awareness] Register client with location when connect
to cluster
..
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master-test.cc@1626
PS6, Line 1626: TestConnectToMasterRegisterLocation
It would be nice to have a scenario to verify how the system behaves in case if
the location-assignment command returns an error.
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master_service.cc@536
PS6, Line 536: resp->set_client_location(location);
generic/paranoid nit: if GetClientLocation() didn't return Status::OK(), I
think we should not try to set the location field in the response. I think
that's one of the best practices if you think of the GetClientLocation() as a
black box and don't have an assurance that the 'location' out argument is not
touched in case of non-OK return code from the function.
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.cc@410
PS6, Line 410: if (!location_mapping_cmd.empty()) {
style code/readability nit: prefer the 'return early' style here, i.e.
if (location_mapping_cmd.empty()) {
return Status::OK();
}
// Continue with the logic specific to the process of running the location
command
...
You can get more details about this at:
https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.cc@415
PS6, Line 415: if (s.ok()) {
: *location = std::move(location_temp);
: } else {
: KLOG_EVERY_N_SECS(ERROR, 60) << Substitute(
: "Unable to assign location to client: $0",
: registration.rpc_addresses(0).host());
: return s;
: }
the same style nit: prefer the 'return early' style
--
To view, visit http://gerrit.cloudera.org:8080/11923
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a
Gerrit-Change-Number: 11923
Gerrit-PatchSet: 6
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Fengling Wang
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 20 Nov 2018 21:19:00 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11923 )
Change subject: [location_awareness] Register client with location when connect
to cluster
..
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/client/client-internal.cc@714
PS6, Line 714: location_ = connect_response.client_location();
Does location_ need to be protected by leader_master_lock_? Why or why not?
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/master-test.cc@1635
PS6, Line 1635: ASSERT_OK(proxy_->ConnectToMaster(req, , ));
Should also ASSERT that !resp.has_error.
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.h
File src/kudu/master/ts_descriptor.h:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.h@182
PS6, Line 182: class ClientDescriptor {
Why is GetClientLocation stored in this class? Do you envision adding more
content to it in the near future?
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11923/6/src/kudu/master/ts_descriptor.cc@411
PS6, Line 411: string location_temp;
I don't think this indirection is needed; GetLocationFromLocationMappingCmd
won't modify the third parameter if it fails, so you could feed 'location'
directly into it.
--
To view, visit http://gerrit.cloudera.org:8080/11923
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a
Gerrit-Change-Number: 11923
Gerrit-PatchSet: 6
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Fengling Wang
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 20 Nov 2018 00:22:19 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11923 to look at the new patch set (#6). Change subject: [location_awareness] Register client with location when connect to cluster .. [location_awareness] Register client with location when connect to cluster This patch enables the client to get a location assigned after connected to the cluster. After that, the client can compare its location with the replica locations to select which replica to read. Change-Id: I0efb327293d86168a30b05305f69d011ad15587a --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/master/ts_descriptor.cc M src/kudu/master/ts_descriptor.h 10 files changed, 97 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/11923/6 -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 6 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Register client with location when connect to cluster
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master_service.cc@537 PS5, Line 537: Status s = TSDescriptor::RegisterNew(instance, registration, ); > As far as I remember, the idea was to assign location for the client based I've been trying to write a separate function to do the client location assignment. But there are dependencies (the defn of location_mapping_cmd, GetLocationFromLocationMappingCmd helper function, etc..) all in the ts_descriptor. I'm thinking maybe move these common parts that do location assignment in a new file so that the master_service can be depended on the new file without depending on the ts_descriptor. Does that sound reasonable? -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Thu, 15 Nov 2018 07:49:03 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/11923/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11923/5//COMMIT_MSG@10 PS5, Line 10: After that, the client can compare its : location with the replica locations to select which replica to read > I'm planning to. Would you suggest it to be in this patch or a separate one Yes, I would expect this to be in a separate changelist. -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 14 Nov 2018 01:52:37 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/11923/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11923/5//COMMIT_MSG@10 PS5, Line 10: After that, the client can compare its : location with the replica locations to select which replica to read > Just to clarify: that's going to be published as a separate changelist, rig I'm planning to. Would you suggest it to be in this patch or a separate one? -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 13 Nov 2018 23:59:59 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/11923/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11923/5//COMMIT_MSG@10 PS5, Line 10: After that, the client can compare its : location with the replica locations to select which replica to read Just to clarify: that's going to be published as a separate changelist, right? http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master-test.cc@1627 PS5, Line 1627: const string kLocationCmdPath = JoinPathSegments(GetTestExecutableDirectory(), : "testdata/first_argument.sh"); A generic note regarding the location scripts: once a test starts using a helper script, it's necessary to update its entry in CMakeLists.txt to include the script into the DATA_FILES. In other words, once a test becomes depending on some file, that dependency should be tracked. You could take a look at the entry for ts_descriptor-test as an example. -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 13 Nov 2018 22:01:27 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master.proto@677 PS5, Line 677: optional string client_location = 7; nit: add a comment describing what client location is http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master_service.cc@537 PS5, Line 537: Status s = TSDescriptor::RegisterNew(instance, registration, ); > I see. I thought the client should also get location assigned through regis As far as I remember, the idea was to assign location for the client based on its IP address using the location assignment script. However, tserver registration is not relevant here. Basically, the tserver registration code is very specific for registering tablet servers, not assigning location to a client. In general, I think here we need to pass information on client IP address to the location assignment script and get the result, but that should not go through TSDescriptor-related code path since it has too much of tserver-related specifics. -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 13 Nov 2018 21:35:55 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master_service.cc@537 PS5, Line 537: Status s = TSDescriptor::RegisterNew(instance, registration, ); > This seems like the most significant aspect of the change, and should be pu I see. I thought the client should also get location assigned through registration. But maybe not? -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 13 Nov 2018 19:30:01 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/11923/5/src/kudu/master/master_service.cc@537 PS5, Line 537: Status s = TSDescriptor::RegisterNew(instance, registration, ); This seems like the most significant aspect of the change, and should be pulled out into its own commit so it can be properly discussed. What's the motivation for treating the client as a tserver and "registering" it with the master vs. refactoring the location-assigning machinery so that clients can use it without registering? -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 13 Nov 2018 19:14:27 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Register client with location when connect to cluster
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11923 to look at the new patch set (#5). Change subject: [location_awareness] Register client with location when connect to cluster .. [location_awareness] Register client with location when connect to cluster This patch enables the client to get a location assigned after connected to the cluster. After that, the client can compare its location with the replica locations to select which replica to read. Change-Id: I0efb327293d86168a30b05305f69d011ad15587a --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc 8 files changed, 75 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/11923/5 -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [location awareness] Register client with location when connect to cluster
Fengling Wang has restored this change. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. Restored -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: restore Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 4 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [location awareness] Register client with location when connect to cluster
Fengling Wang has abandoned this change. ( http://gerrit.cloudera.org:8080/11923 ) Change subject: [location_awareness] Register client with location when connect to cluster .. Abandoned I'll start a new one. -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 4 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [location awareness] Register client with location when connect to cluster
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11923 to look at the new patch set (#4). Change subject: [location_awareness] Register client with location when connect to cluster .. [location_awareness] Register client with location when connect to cluster This patch enables the client to get a location assigned after connected to the cluster. After that, the client can compare its location with the replica locations to select which replica to read. Change-Id: I0efb327293d86168a30b05305f69d011ad15587a --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc 8 files changed, 75 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/11923/4 -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 4 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [location awareness] Register client with location when connect to cluster
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11923 to look at the new patch set (#3). Change subject: [location_awareness] Register client with location when connect to cluster .. [location_awareness] Register client with location when connect to cluster This patch enables the client to get a location assigned after connected to the cluster. After that, the client can compare its location with the replica locations to select which replica to read. Change-Id: I0efb327293d86168a30b05305f69d011ad15587a --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc 8 files changed, 75 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/11923/3 -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 3 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [location awareness] Register client with location when connect to cluster
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11923 to look at the new patch set (#2). Change subject: [location_awareness] Register client with location when connect to cluster .. [location_awareness] Register client with location when connect to cluster Change-Id: I0efb327293d86168a30b05305f69d011ad15587a --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc 8 files changed, 75 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/11923/2 -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 2 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [location awareness] Register client with location when connect to cluster
Fengling Wang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11923 Change subject: [location_awareness] Register client with location when connect to cluster .. [location_awareness] Register client with location when connect to cluster Change-Id: I0efb327293d86168a30b05305f69d011ad15587a --- M src/kudu/client/client-internal.cc M src/kudu/client/client-internal.h M src/kudu/client/client-test.cc M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/master/master-test.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc 8 files changed, 76 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/11923/1 -- To view, visit http://gerrit.cloudera.org:8080/11923 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I0efb327293d86168a30b05305f69d011ad15587a Gerrit-Change-Number: 11923 Gerrit-PatchSet: 1 Gerrit-Owner: Fengling Wang
