[kudu-CR] [location awareness] Add ts location to both client and internal client
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: [location_awareness] Add ts location to both client and internal client > Thanks, Adar and Alexey for the suggestions! One thing I'm not very clear, Ah, it seems a typo in my last comment was the source of the confusion. I meant '[client] expose location in KuduTabletServer via private API'. Maybe, a better wording might be: '[client] expose location via internal API' And then add the detailed description. And yes, since the location is needed for both KuduTabletServer and RemoteTabletServer, it's needed for both, I think. -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 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: Wed, 24 Oct 2018 05:23:14 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: Add ts location to both client and internal client > Thank you for the clarification, Adar! Thanks, Adar and Alexey for the suggestions! One thing I'm not very clear, so do I expose location via private API for both KuduTabletServer and RemoteTabletServer? -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 2 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, 24 Oct 2018 04:36:46 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: Add ts location to both client and internal client > I wasn't aware of the RemoteKsckCluster use case. Given that, I agree that Thank you for the clarification, Adar! I agree it's crucial to have the right wording in the commit message regarding the change. Fengling, could you address that? I think something like '[client] expose location in RemoteTabletServer via private API' could be good enough summary (i.e. the first line of the commit message). -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 2 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, 23 Oct 2018 21:34:56 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: [location_awareness] Add ts location to both client and internal client > Probably, that has already been discussed, and if so, I just want to have i I wasn't aware of the RemoteKsckCluster use case. Given that, I agree that exposing location in KuduTabletServer with KUDU_NO_EXPORT makes the most sense. That said, this commit message should avoid saying that the ts location is being added to the public API. -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 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, 23 Oct 2018 20:35:20 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: Add ts location to both client and internal client > That make sense, but then I don't think you need to expose location() in cl Probably, that has already been discussed, and if so, I just want to have it in the review as well. It seems the idea was to use it in https://gerrit.cloudera.org/#/c/11422/11/src/kudu/tools/ksck_remote.cc@487 Is that acceptable to have it exposed in this patch or we want to have a separate patch with exposing the location via kudu::client::KuduTabletServer API (not exported, though)? Or another alternative would be not exposing the location via a public method of KuduTabletServer (but in internal-only API); instead, befriend RemoteKsckCluster and access the location field in src/kudu/tools/ksck_remote.cc via the KuduTabletServer::Data class? Adar, any preference on the items above? -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 2 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, 23 Oct 2018 20:24:10 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11679 to look at the new patch set (#6). Change subject: [location_awareness] Add ts location to both client and internal client .. [location_awareness] Add ts location to both client and internal client I added location info to client::KuduTabletServer and client::internal::RemoteTabletServer to enable the future work on location-aware C++ and Java clients. Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac --- M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc M src/kudu/client/tablet_server-internal.cc M src/kudu/client/tablet_server-internal.h 7 files changed, 29 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/11679/6 -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 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] Add ts location to both client and internal client
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11679 to look at the new patch set (#5). Change subject: [location_awareness] Add ts location to both client and internal client .. [location_awareness] Add ts location to both client and internal client I added location info to client::KuduTabletServer and client::internal::RemoteTabletServer to enable the future work on location-aware C++ and Java clients. Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac --- M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc M src/kudu/client/tablet_server-internal.cc M src/kudu/client/tablet_server-internal.h M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/scripts/first_argument.sh M src/kudu/integration-tests/table_locations-itest.cc M src/kudu/master/catalog_manager.cc 11 files changed, 164 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/11679/5 -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 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
[kudu-CR] [location awareness] Add ts location to both client and internal client
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: [location_awareness] Add ts location to both client and internal client > There will be future work of client choosing which replica to read based on That make sense, but then I don't think you need to expose location() in client.h at all; simply storing it in one of the ::Data classes would probably suffice. -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 4 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, 18 Oct 2018 23:53:45 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: Add ts location to both client and internal client > As-is, even though you've added a new accessor with public visibility to cl There will be future work of client choosing which replica to read based on the replicas' locations. So we need to keep a record of the replicas with location accessible in the client. Will, would you mind to see if what I said makes sense? -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 2 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, 18 Oct 2018 23:11:05 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: [location_awareness] Add ts location to both client and internal client > I'm not sure if I understand. We make it a private API so that the client c As-is, even though you've added a new accessor with public visibility to client.h, the C++ public client API surface has not changed, because the accessor is annotated with KUDU_NO_EXPORT, which means third party applications can't call it. So what's the broader context here? How will this patch be used? -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 4 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, 18 Oct 2018 22:29:06 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 4: I made a separate server-side patch https://gerrit.cloudera.org/#/c/11727/ , which this client-side patch is rebased on. -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 4 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, 18 Oct 2018 21:18:23 + Gerrit-HasComments: No
[kudu-CR] [location awareness] Add ts location to both client and internal client
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11679 ) Change subject: [location_awareness] Add ts location to both client and internal client .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@7 PS2, Line 7: Add ts location to both client and internal client > This isn't quite correct though because you've annotated location() in clie I'm not sure if I understand. We make it a private API so that the client can be location-aware and this API is not necessarily public? http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@11 PS2, Line 11: RemoteTabletServer > This itself is a client-side class that gets its info from TSInfoPB so I th Done http://gerrit.cloudera.org:8080/#/c/11679/2//COMMIT_MSG@14 PS2, Line 14: So I added the location into them in order to ensure the consistency > nit: Unnecessary line break. Done http://gerrit.cloudera.org:8080/#/c/11679/2/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/11679/2/src/kudu/client/client.h@631 PS2, Line 631: const KUDU_NO_EXPORT std::string& location() const; > For functions, KUDU_NO_EXPORT has to come at the end. See commit ed60c11a3 I see. Thanks. -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 2 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, 18 Oct 2018 21:15:55 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add ts location to both client and internal client
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11679 to look at the new patch set (#4). Change subject: [location_awareness] Add ts location to both client and internal client .. [location_awareness] Add ts location to both client and internal client I added location info to client::KuduTabletServer and client::internal::RemoteTabletServer to enable the future work on location-aware C++ and Java clients. Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac --- M src/kudu/client/client.cc M src/kudu/client/client.h M src/kudu/client/meta_cache.cc M src/kudu/client/meta_cache.h M src/kudu/client/scan_token-internal.cc M src/kudu/client/tablet_server-internal.cc M src/kudu/client/tablet_server-internal.h 7 files changed, 29 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/79/11679/4 -- To view, visit http://gerrit.cloudera.org:8080/11679 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib92cc6806073d32c859ae44ff803abb37cac99ac Gerrit-Change-Number: 11679 Gerrit-PatchSet: 4 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
