You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2020/09/23 03:12:12 UTC
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16493
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
KUDU-3192: [client] Expose the cluster ID in the client KuduTable
This patch adds a cluster ID field to the KuduTable object in the
C++ and Java Kudu clients. This field will be used in a follow on
change to better enable the HMS integration to use the cluster
ID.
Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKeyEncoding.java
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/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/table-internal.cc
M src/kudu/client/table-internal.h
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
22 files changed, 115 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/16493/1
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16493
to look at the new patch set (#2).
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
KUDU-3192: [client] Expose the cluster ID in the client KuduTable
This patch adds a cluster ID field to the KuduTable object in the
C++ and Java Kudu clients. This field will be used in a follow on
change to better enable the HMS integration to use the cluster
ID.
Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKeyEncoding.java
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/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/table-internal.cc
M src/kudu/client/table-internal.h
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
22 files changed, 118 insertions(+), 25 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/16493/2
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
KUDU-3192: [client] Expose the cluster ID in the client KuduTable
This patch adds a cluster ID field to the C++ and Java Kudu
clients. This field will be used in a follow on change to better
enable the HMS integration to use the cluster ID.
Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Reviewed-on: http://gerrit.cloudera.org:8080/16493
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Grant Henke <gr...@apache.org>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
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/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
14 files changed, 119 insertions(+), 15 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved
Grant Henke: Verified
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16493
to look at the new patch set (#3).
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
KUDU-3192: [client] Expose the cluster ID in the client KuduTable
This patch adds a cluster ID field to the C++ and Java Kudu
clients. This field will be used in a follow on change to better
enable the HMS integration to use the cluster ID.
Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
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/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
14 files changed, 118 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/16493/3
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16493/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16493/2//COMMIT_MSG@9
PS2, Line 9: This patch adds a cluster ID field to the KuduTable object in the
I'm surprised this field has to be associated with Kudu tables instead of entire Kudu clients, which get initialized with a single cluster via the master addresses. I understand that it will be a per table field in the HMS, but I'm not convinced that means it should be a public API for tables.
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Sep 2020 17:44:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16493/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16493/2//COMMIT_MSG@9
PS2, Line 9: This patch adds a cluster ID field to the KuduTable object in the
> It is convenient to have it on the KuduTable object so that the user can ch
I agree it's convenient to know the cluster ID, but every table will hit the same masters and get the same cluster ID. It seems like if we have a KuduTable, we should also have a Kudu client, in which case we could get the cluster ID directly from the client for such validation. I'd be onboard with the ConnectToMaster approach.
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Sep 2020 18:13:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16493
to look at the new patch set (#4).
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
KUDU-3192: [client] Expose the cluster ID in the client KuduTable
This patch adds a cluster ID field to the C++ and Java Kudu
clients. This field will be used in a follow on change to better
enable the HMS integration to use the cluster ID.
Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
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/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/master_failover-itest.cc
M src/kudu/master/master-test.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
14 files changed, 119 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/16493/4
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Patch Set 2: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Sep 2020 13:15:20 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Removed Code-Review+2 by Andrew Wong <aw...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16493/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/16493/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1328
PS3, Line 1328: assertTrue(client.getClusterId().isEmpty());
: // Do something that will cause the client to connect to the cluster.
: client.listTabletServers();
: assertFalse(client.getClusterId().isEmpty());
nit: maybe assert the strings are equal?
http://gerrit.cloudera.org:8080/#/c/16493/3/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:
http://gerrit.cloudera.org:8080/#/c/16493/3/src/kudu/integration-tests/cluster_itest_util.h@465
PS3, Line 465: const MonoDelta& timeout,
: std::string* cluster_id);
nit: spacing
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 29 Sep 2020 19:31:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Removed Code-Review+2 by Andrew Wong <aw...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Sep 2020 18:15:36 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Patch Set 4: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 29 Sep 2020 21:15:19 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Patch Set 2:
Mobile gerrit makes it too easy to +2 things :)
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Sep 2020 18:16:04 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16493/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/16493/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1328
PS3, Line 1328: assertTrue(client.getClusterId().isEmpty());
: // Do something that will cause the client to connect to the cluster.
: client.listTabletServers();
: assertFalse(client.getClusterId().isEmpty());
> nit: maybe assert the strings are equal?
It is auto-generated server side and there isn't a great way to get it from the server (without using the RPC this is testing).
http://gerrit.cloudera.org:8080/#/c/16493/3/src/kudu/integration-tests/cluster_itest_util.h
File src/kudu/integration-tests/cluster_itest_util.h:
http://gerrit.cloudera.org:8080/#/c/16493/3/src/kudu/integration-tests/cluster_itest_util.h@465
PS3, Line 465: const MonoDelta& timeout,
: std::string* cluster_id);
> nit: spacing
Done
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 29 Sep 2020 20:31:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16493/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16493/2//COMMIT_MSG@9
PS2, Line 9: This patch adds a cluster ID field to the KuduTable object in the
> I'm surprised this field has to be associated with Kudu tables instead of e
It is convenient to have it on the KuduTable object so that the user can check if the table is from the cluster they expect. It could also be useful if there are applications that talk to multiple clusters.
I could add cluster ID to the ConnectToMasterResponsePB and store it in the client as well when it connects, but I still might consider adding a convenience method to the client that returns client.getClusterId using the client in the KuduTable object.
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Sep 2020 17:57:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3192: [client] Expose the cluster ID in the client KuduTable
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16493 )
Change subject: KUDU-3192: [client] Expose the cluster ID in the client KuduTable
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16493/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java:
http://gerrit.cloudera.org:8080/#/c/16493/3/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@1328
PS3, Line 1328: assertTrue(client.getClusterId().isEmpty());
: // Do something that will cause the client to connect to the cluster.
: client.listTabletServers();
: assertFalse(client.getClusterId().isEmpty());
> It is auto-generated server side and there isn't a great way to get it from
Ugh, I totally misread this test -- as you deduced I was referring to the cluster ID before the client even connected to the cluster, which I agree is fine to not test, given that's tested elsewhere.
--
To view, visit http://gerrit.cloudera.org:8080/16493
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia1f5a451aaa44834534d2387ee1c9aa9cf95dd37
Gerrit-Change-Number: 16493
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 29 Sep 2020 20:47:45 +0000
Gerrit-HasComments: Yes