You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Yao Xu (Code Review)" <ge...@cloudera.org> on 2019/07/24 07:15:47 UTC

[kudu-CR] Deprecated TabletLocationsPB.ReplicaPB message 

Yao Xu has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13908


Change subject: Deprecated TabletLocationsPB.ReplicaPB message
......................................................................

Deprecated TabletLocationsPB.ReplicaPB message

According to the comments of https://gerrit.cloudera.org/#/c/13875/, the
TabletLocationsPB.ReplicaPB message has been deprecated a long time ago.

In this patch, I removed the dependency on it from the code.

Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/GetTableLocationsRequest.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.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/TestRemoteTablet.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java
M src/kudu/client/client.cc
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer_tool-test.cc
28 files changed, 247 insertions(+), 290 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/1
-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 1
Gerrit-Owner: Yao Xu <oc...@gmail.com>

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 9: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/meta_cache.cc@198
PS8, Line 198:     replicas.push_back(replica);
> > Hmm, guess I was wrong about std::move here and below.
Yeah that's right. I thought there was an std::string in there (UUID) but I was wrong.



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 9
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Mon, 29 Jul 2019 18:21:00 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/client.cc
File src/kudu/client/client.cc:

http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/client.cc@582
PS8, Line 582:     // TODO(aserbin): try to use member_type instead of role for metacache.
             :     bool is_leader = r.role() == consensus::RaftPeerPB::LEADER;
             :     bool is_voter = is_leader || r.role() == consensus::RaftPeerPB::FOLLOWER;
             :     unique_ptr<KuduReplica> replica(new KuduReplica);
             :     replica->data_ = new KuduReplica::Data(is_leader, is_voter, std::move(ts));
             : 
             :     replicas.push_back(replica.release());
Seems like we could deduplicate this chunk of code too?


http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/meta_cache.cc@198
PS8, Line 198:     replicas.push_back(std::move(replica));
> warning: std::move of the variable 'replica' of the trivially-copyable type
Hmm, guess I was wrong about std::move here and below.



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 8
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Sun, 28 Jul 2019 21:50:51 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13908

to look at the new patch set (#3).

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................

[client] Deprecated TabletLocationsPB.ReplicaPB message in client

According to the comments of https://gerrit.cloudera.org/#/c/13875/, the
TabletLocationsPB.ReplicaPB message has been deprecated a long time ago.

In this patch, I removed the dependency on it from the client. The server-side
is compatible with both old and new client.

Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.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/TestRemoteTablet.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java
M src/kudu/client/client.cc
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer_tool-test.cc
27 files changed, 266 insertions(+), 232 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/3
-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 3
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13908/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13908/1//COMMIT_MSG@9
PS1, Line 9: According to the comments of https://gerrit.cloudera.org/#/c/13875/, the
> hrm, in those comments, it claimed that ReplicaPB was deprecated as of 0.5.
Whoops, it seems that was my mistake claiming that ReplicaPB was deprecated as of 0.5.0.  It seems I confused the version of the line with DEPRECATED comment with one of the lines of 'message ReplicaPB' definition.  Sorry.



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 1
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 16:50:29 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

PS3: 
> > Doesn't this change mean that old servers (those that don't support
In general we preserve backwards compatibility for "both" combinations (i.e. "new" server with "old" client and "old" server with "new" client). That's why if e.g. we decide to replace one RPC with another, you'll find server-side code that handles both RPCs, and client-side code that looks at server capabilities to decide which RPC to send.

There are exceptions where we've failed to do this, but this is the general policy for Kudu.



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 4
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Thu, 25 Jul 2019 18:47:26 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

PS3: 
> Doesn't this change mean that old servers (those that don't support
 > this interning) won't be able to communicate with new clients? We'd
 > like to preserve that backwards compatibility.

I'm not sure if the new client is compatible with the old server. Maybe we can upgrade server first and then upgrade the client?



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 3
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Thu, 25 Jul 2019 04:09:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13908

to look at the new patch set (#8).

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................

[client] Deprecated TabletLocationsPB.ReplicaPB message in client

In this patch, I removed the dependency on it from the client. It's only
used for backward compatibility with RPC. In the future we will completely
deprecated TabletLocationsPB.ReplicaPB message.

Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.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/TestRemoteTablet.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java
M src/kudu/client/client.cc
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer_tool-test.cc
27 files changed, 319 insertions(+), 234 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/8
-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 8
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

PS3: 
> In general we preserve backwards compatibility for "both"
 > combinations (i.e. "new" server with "old" client and "old" server
 > with "new" client). That's why if e.g. we decide to replace one RPC
 > with another, you'll find server-side code that handles both RPCs,
 > and client-side code that looks at server capabilities to decide
 > which RPC to send.
 > 
 > There are exceptions where we've failed to do this, but this is the
 > general policy for Kudu.

Okay, I understand. I will be backward compatible at both client-side and server-side.



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 6
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Jul 2019 06:47:44 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:

http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/integration-tests/table_locations-itest.cc@250
PS3, Line 250: const string& ta
> Capture by cref if you're not using std::move.
Done


http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/integration-tests/table_locations-itest.cc@275
PS3, Line 275:       NO_FATALS(get_tablet_location(proxy_.get(), loc.tablet_id()));
> Need to wrap in NO_FATALS.
Done


http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/master/master.proto@a565
PS3, Line 565: 
> I think this comment was useful; could you restore it, and duplicate it int
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 4
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Thu, 25 Jul 2019 07:20:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13908

to look at the new patch set (#2).

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................

[client] Deprecated TabletLocationsPB.ReplicaPB message in client

According to the comments of https://gerrit.cloudera.org/#/c/13875/, the
TabletLocationsPB.ReplicaPB message has been deprecated a long time ago.

In this patch, I removed the dependency on it from the client. The server-side
is compatible with both old and new client.

Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.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/TestRemoteTablet.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java
M src/kudu/client/client.cc
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer_tool-test.cc
27 files changed, 266 insertions(+), 232 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/2
-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 2
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................

[client] Deprecated TabletLocationsPB.ReplicaPB message in client

In this patch, I removed the dependency on it from the client. It's only
used for backward compatibility with RPC. In the future we will completely
deprecated TabletLocationsPB.ReplicaPB message.

Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Reviewed-on: http://gerrit.cloudera.org:8080/13908
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.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/TestRemoteTablet.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java
M src/kudu/client/client.cc
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer_tool-test.cc
27 files changed, 315 insertions(+), 236 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Adar Dembo: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 10
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 8:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/client.cc
File src/kudu/client/client.cc:

http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/client.cc@551
PS6, Line 551: 
> Doesn't this need to also handle the case where we're talking to an
 > old server that doesn't provide interned_replicas?

Opps. I forgot to fix this.


http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/meta_cache.cc@196
PS6, Line 196: 
> Nit: could you add /*failed=*/ just before 'false' here and below? It helps
Done


http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/meta_cache.cc@197
PS6, Line 197:     r.r
> std::move
Done


http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/meta_cache.cc@208
PS6, Line 208: = { Fin
> std::move
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 8
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Sat, 27 Jul 2019 05:36:06 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13908

to look at the new patch set (#4).

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................

[client] Deprecated TabletLocationsPB.ReplicaPB message in client

According to the comments of https://gerrit.cloudera.org/#/c/13875/, the
TabletLocationsPB.ReplicaPB message has been deprecated a long time ago.

In this patch, I removed the dependency on it from the client. The server-side
is compatible with both old and new client.

Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.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/TestRemoteTablet.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java
M src/kudu/client/client.cc
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer_tool-test.cc
27 files changed, 272 insertions(+), 231 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/4
-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 4
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13908

to look at the new patch set (#6).

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................

[client] Deprecated TabletLocationsPB.ReplicaPB message in client

In this patch, I removed the dependency on it from the client. It's only
used for backward compatibility with RPC. In the future we will completely
deprecated TabletLocationsPB.ReplicaPB message.

Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.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/TestRemoteTablet.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java
M src/kudu/client/client.cc
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer_tool-test.cc
27 files changed, 297 insertions(+), 233 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/6
-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 6
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>

[kudu-CR] Deprecated TabletLocationsPB.ReplicaPB message

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: Deprecated TabletLocationsPB.ReplicaPB message
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/13908/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13908/1//COMMIT_MSG@9
PS1, Line 9: According to the comments of https://gerrit.cloudera.org/#/c/13875/, the
hrm, in those comments, it claimed that ReplicaPB was deprecated as of 0.5.0, but I'm not sure where that came from. The new InternedReplicaPB was added in 586e957f76a547340f2ab93a7eebc3f116ff825e which was only a few months ago. I think removing ReplicaPB would break wire compatibility with 1.9 and below?



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 1
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jul 2019 07:19:38 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 6:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/client.cc
File src/kudu/client/client.cc:

http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/client.cc@551
PS6, Line 551:   for (const auto& r : t.interned_replicas()) {
Doesn't this need to also handle the case where we're talking to an old server that doesn't provide interned_replicas?


http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/meta_cache.cc@196
PS6, Line 196: false
Nit: could you add /*failed=*/ just before 'false' here and below? It helps readers understand the meaning of this field.


http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/meta_cache.cc@197
PS6, Line 197: replica
std::move


http://gerrit.cloudera.org:8080/#/c/13908/6/src/kudu/client/meta_cache.cc@208
PS6, Line 208: replica
std::move



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 6
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Fri, 26 Jul 2019 18:35:23 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13908

to look at the new patch set (#7).

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................

[client] Deprecated TabletLocationsPB.ReplicaPB message in client

In this patch, I removed the dependency on it from the client. It's only
used for backward compatibility with RPC. In the future we will completely
deprecated TabletLocationsPB.ReplicaPB message.

Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.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/TestRemoteTablet.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java
M src/kudu/client/client.cc
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer_tool-test.cc
27 files changed, 319 insertions(+), 234 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/7
-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 7
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 9:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/client.cc
File src/kudu/client/client.cc:

http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/client.cc@582
PS8, Line 582:     RETURN_NOT_OK(add_replica_func(resp.ts_infos(r.ts_info_idx()), r.role(), &replicas));
             :   }
             : 
             :   unique_ptr<KuduTablet> client_tablet(new KuduTablet);
             :   client_tablet->data_ = new KuduTablet::Data(tablet_id, std::move(replicas));
             :   replicas.clear();
             : 
> Seems like we could deduplicate this chunk of code too?
Done


http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

http://gerrit.cloudera.org:8080/#/c/13908/8/src/kudu/client/meta_cache.cc@198
PS8, Line 198:     replicas.push_back(replica);
> Hmm, guess I was wrong about std::move here and below.

Emm, I think because RemoteReplica's members are pointers and enumerations, so there's no improvement in using std::move. Is that right?



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 9
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Mon, 29 Jul 2019 05:53:30 +0000
Gerrit-HasComments: Yes

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13908

to look at the new patch set (#9).

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................

[client] Deprecated TabletLocationsPB.ReplicaPB message in client

In this patch, I removed the dependency on it from the client. It's only
used for backward compatibility with RPC. In the future we will completely
deprecated TabletLocationsPB.ReplicaPB message.

Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.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/TestRemoteTablet.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java
M src/kudu/client/client.cc
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer_tool-test.cc
27 files changed, 315 insertions(+), 236 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/9
-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 9
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................


Patch Set 3:

(4 comments)

Echoing Todd's comments, we don't want to break backwards or forwards compatibility, and I think the changes in meta_cache.cc break compat between old servers and new clients.

You might consider splitting up the patch too; there are a number of related but not tightly coupled concerns here (i.e. using ReplicaPB internally, using it on the wire, interning replicas in GetTabletLocationsResponse, etc.)

http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

PS3: 
Doesn't this change mean that old servers (those that don't support this interning) won't be able to communicate with new clients? We'd like to preserve that backwards compatibility.


http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:

http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/integration-tests/table_locations-itest.cc@250
PS3, Line 250: string tablet_id
Capture by cref if you're not using std::move.


http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/integration-tests/table_locations-itest.cc@275
PS3, Line 275:       get_tablet_location(proxy_.get(), loc.tablet_id());
Need to wrap in NO_FATALS.


http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/13908/3/src/kudu/master/master.proto@a565
PS3, Line 565: 
I think this comment was useful; could you restore it, and duplicate it into GetTabletLocationsResponsePB?



-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 3
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 17:49:02 +0000
Gerrit-HasComments: Yes

[kudu-CR] Deprecated TabletLocationsPB.ReplicaPB message

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Yao Xu has posted comments on this change. ( http://gerrit.cloudera.org:8080/13908 )

Change subject: Deprecated TabletLocationsPB.ReplicaPB message
......................................................................


Patch Set 1:

> (1 comment)

It seems like this. :(
I think I can make some changes to make the server-side compatible with the old client.


-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 1
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Wed, 24 Jul 2019 07:31:43 +0000
Gerrit-HasComments: No

[kudu-CR] [client] Deprecated TabletLocationsPB.ReplicaPB message in client

Posted by "Yao Xu (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/13908

to look at the new patch set (#5).

Change subject: [client] Deprecated TabletLocationsPB.ReplicaPB message in client
......................................................................

[client] Deprecated TabletLocationsPB.ReplicaPB message in client

In this patch, I removed the dependency on it from the client. It's only
used for backward compatibility with RPC. In the future we will completely
deprecated TabletLocationsPB.ReplicaPB message.

Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToClusterResponse.java
M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.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/TestRemoteTablet.java
M java/kudu-test-utils/src/main/java/org/apache/kudu/test/ProtobufUtils.java
M src/kudu/client/client.cc
M src/kudu/client/meta_cache.cc
M src/kudu/integration-tests/cluster_itest_util.cc
M src/kudu/integration-tests/cluster_itest_util.h
M src/kudu/integration-tests/delete_table-itest.cc
M src/kudu/integration-tests/linked_list-test.cc
M src/kudu/integration-tests/master_sentry-itest.cc
M src/kudu/integration-tests/raft_config_change-itest.cc
M src/kudu/integration-tests/raft_consensus-itest.cc
M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc
M src/kudu/integration-tests/registration-test.cc
M src/kudu/integration-tests/table_locations-itest.cc
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/integration-tests/tombstoned_voting-itest.cc
M src/kudu/integration-tests/ts_itest-base.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
M src/kudu/master/master_service.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tools/rebalancer_tool-test.cc
27 files changed, 297 insertions(+), 232 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/13908/5
-- 
To view, visit http://gerrit.cloudera.org:8080/13908
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I18272274f07d5ae8e9f6b9572f9900aa8df27bef
Gerrit-Change-Number: 13908
Gerrit-PatchSet: 5
Gerrit-Owner: Yao Xu <oc...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>