You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2018/12/28 21:44:44 UTC

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client 

Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12138


Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................

Support location awareness in READ_CLOSEST for the C++ client

Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
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/integration-tests/CMakeLists.txt
A src/kudu/integration-tests/location_assignment-itest.cc
D src/kudu/integration-tests/ts_location_assignment-itest.cc
9 files changed, 282 insertions(+), 157 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................

Support location awareness in READ_CLOSEST for the C++ client

Previously, in READ_CLOSEST, the C++ client would choose to read from a
local tablet server if possible and otherwise pick a random replica.
This changes and enhances READ_CLOSEST mode to work as follows:

1. If there is a local server, use it. If there are multiple local
   servers, choose one at random.
2. If there is a server in the same location as the client, use it. If
   there are multiple servers in the same location, choose one at
   random.
3. Otherwise, choose a server at random.

This is not a breaking change, as in the absence of locations the
behavior is consistent with what was documented before, which was only
that a local server would be chosen if possible, else a random server.

Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
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/integration-tests/location_assignment-itest.cc
7 files changed, 153 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 7:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@406
PS7, Line 406: random
> nit: does it make sense to add a TODO for the future to make use of the loc
Done


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@408
PS7, Line 408:       vector<RemoteTabletServer*> local;
             :       vector<RemoteTabletServer*> same_location;
> nit: call reserve(filtered.size()) for local and same_location?
Done


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@414
PS7, Line 414:         const string replica_location = rts->location();
> nit: maybe, gate computing/calling this based on !client_location.empty()?
Done


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@422
PS7, Line 422: rand
> Do we init random generator in the client library?  I guess my question is 
After looking at this with Alexey, we think we don't seed rand(), but we should be. He said he would look at it as a separate changelist.


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc
File src/kudu/integration-tests/location_assignment-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc@79
PS7, Line 79: ThreadSafeRandom rng(SeedRandom());
> nit: it seems TabletServerIntegrationTestBase has ThreadSafeRandom as aggre
Done


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc@82
PS7, Line 82:     auto location = Substitute("/L$0", i);
> Now unused.
Done


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc@125
PS7, Line 125:     // When running on Linux, the client and tablet servers each have their own
             :     // IP in the local address space, so no tablet server will be considered
             :     // local to the client. If there is a tablet server in the same location as
             :     // the client, it will be the only tablet server scanned. Otherwise, some
             :     // random tablet server will be scanned.
> nit: move this under the #if defined block below?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 18:58:03 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................

Support location awareness in READ_CLOSEST for the C++ client

Previously, in READ_CLOSEST, the C++ client would choose to read from a
local tablet server if possible and otherwise pick a random replica.
This changes and enhances READ_CLOSEST mode to work as follows:

1. If there is a local server, use it. If there are multiple local
   servers, choose one at random.
2. If there is a server in the same location as the client, use it. If
   there are multiple servers in the same location, choose one at
   random.
3. Otherwise, choose a server at random.

This is not a breaking change, as in the absence of locations the
behavior is consistent with what was documented before, which was only
that a local server would be chosen if possible, else a random server.

Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
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/integration-tests/location_assignment-itest.cc
7 files changed, 147 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client-internal.cc@412
PS1, Line 412:           local.emplace_back(rts);
> Nit: emplace_back for new code.
Done


http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client.h@472
PS1, Line 472:                       ///< Local replicas are considered the closest,
             :                       ///< followed by replicas in the same location as the
             :                       ///< client, followed by all other replicas. If there are
             :                       ///< multiple closest replicas, one is chosen randomly.
> Would you consider this to be a breaking change? The answer and justificati
Done


http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/meta_cache.h
File src/kudu/client/meta_cache.h:

http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/meta_cache.h@109
PS1, Line 109:   // Return a copy of this tablet server's location, as assigned by the master.
> The changes here (removal of constness, returning a copy) suggest that the 
No, the location can change from the point of view of the client. This might happen, for example, if the cluster configuration is changed during the lifetime of a long-lived client instance.

Once the server assigns a location to a tablet server, the location will not change unless the tablet server re-registers, which AFAIK happens only when the tablet server restarts, or if the master restarts and the tablet server needs to register with the new master process. The location could also change if the leader master changes and the location mapping command gave different results on the new leader master than on the old one for the tserver, but this is really a configuration problem.


http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/integration-tests/location_assignment-itest.cc
File src/kudu/integration-tests/location_assignment-itest.cc:

PS1: 
> Could you rename this in a separate commit? It's showing up as a brand new 
Done


http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/integration-tests/location_assignment-itest.cc@51
PS1, Line 51: 
> warning: using decl 'KuduClientBuilder' is unused [misc-unused-using-decls]
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 04 Jan 2019 19:48:34 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has removed Kudu Jenkins from this change.  ( http://gerrit.cloudera.org:8080/12138 )

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Removed reviewer Kudu Jenkins with the following votes:

* Verified-1 by Kudu Jenkins (120)
-- 
To view, visit http://gerrit.cloudera.org:8080/12138
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................

Support location awareness in READ_CLOSEST for the C++ client

Previously, in READ_CLOSEST, the C++ client would choose to read from a
local tablet server if possible and otherwise pick a random replica.
This changes and enhances READ_CLOSEST mode to work as follows:

1. If there is a local server, use it. If there are multiple local
   servers, choose one at random.
2. If there is a server in the same location as the client, use it. If
   there are multiple servers in the same location, choose one at
   random.
3. Otherwise, choose a server at random.

This is not a breaking change, as in the absence of locations the
behavior is consistent with what was documented before, which was only
that a local server would be chosen if possible, else a random server.

Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
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/integration-tests/CMakeLists.txt
M src/kudu/integration-tests/location_assignment-itest.cc
8 files changed, 59 insertions(+), 25 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Adar Dembo, 

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

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

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................

Support location awareness in READ_CLOSEST for the C++ client

Previously, in READ_CLOSEST, the C++ client would choose to read from a
local tablet server if possible and otherwise pick a random replica.
This changes and enhances READ_CLOSEST mode to work as follows:

1. If there is a local server, use it. If there are multiple local
   servers, choose one at random.
2. If there is a server in the same location as the client, use it. If
   there are multiple servers in the same location, choose one at
   random.
3. Otherwise, choose a server at random.

This is not a breaking change, as in the absence of locations the
behavior is consistent with what was documented before, which was only
that a local server would be chosen if possible, else a random server.

Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
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/integration-tests/location_assignment-itest.cc
7 files changed, 150 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc
File src/kudu/integration-tests/location_assignment-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc@82
PS7, Line 82:     auto location = Substitute("/L$0", i);
Now unused.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 05:24:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12138/2/src/kudu/integration-tests/location_assignment-itest.cc
File src/kudu/integration-tests/location_assignment-itest.cc:

PS2: 
> The diff here is weird; it's just include changes. I thought there was also
:facepalm:



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 04 Jan 2019 22:05:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

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

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

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................

Support location awareness in READ_CLOSEST for the C++ client

Previously, in READ_CLOSEST, the C++ client would choose to read from a
local tablet server if possible and otherwise pick a random replica.
This changes and enhances READ_CLOSEST mode to work as follows:

1. If there is a local server, use it. If there are multiple local
   servers, choose one at random.
2. If there is a server in the same location as the client, use it. If
   there are multiple servers in the same location, choose one at
   random.
3. Otherwise, choose a server at random.

This is not a breaking change, as in the absence of locations the
behavior is consistent with what was documented before, which was only
that a local server would be chosen if possible, else a random server.

Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
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/integration-tests/location_assignment-itest.cc
7 files changed, 156 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@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: Will Berkeley <wd...@gmail.com>

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................

Support location awareness in READ_CLOSEST for the C++ client

Previously, in READ_CLOSEST, the C++ client would choose to read from a
local tablet server if possible and otherwise pick a random replica.
This changes and enhances READ_CLOSEST mode to work as follows:

1. If there is a local server, use it. If there are multiple local
   servers, choose one at random.
2. If there is a server in the same location as the client, use it. If
   there are multiple servers in the same location, choose one at
   random.
3. Otherwise, choose a server at random.

This is not a breaking change, as in the absence of locations the
behavior is consistent with what was documented before, which was only
that a local server would be chosen if possible, else a random server.

Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
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/integration-tests/location_assignment-itest.cc
7 files changed, 153 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client-internal.cc@412
PS1, Line 412:           local.push_back(rts);
Nit: emplace_back for new code.


http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client.h
File src/kudu/client/client.h:

http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client.h@472
PS1, Line 472:                       ///< Local replicas are considered the closest,
             :                       ///< followed by replicas in the same location as the
             :                       ///< client, followed by all other replicas. If there are
             :                       ///< multiple closest replicas, one is chosen randomly.
Would you consider this to be a breaking change? The answer and justification would be good to add to the commit message.


http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/meta_cache.h
File src/kudu/client/meta_cache.h:

http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/meta_cache.h@109
PS1, Line 109:   // Return a copy of this tablet server's location, as assigned by the master.
The changes here (removal of constness, returning a copy) suggest that the location of a tserver is no longer immutable. That's true to the extent that a location could change from empty (i.e. unassigned) to "some location", but once a location has been assigned, it may not change to some other location, right?

As written such a change is legal, but that isn't allowed by the server, right?


http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/integration-tests/location_assignment-itest.cc
File src/kudu/integration-tests/location_assignment-itest.cc:

PS1: 
Could you rename this in a separate commit? It's showing up as a brand new file so it's hard to review just the changes.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 02 Jan 2019 18:16:05 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12138/1/src/kudu/client/client-internal.cc@412
PS1, Line 412:           local.emplace_back(rts);
> Done
Below too (L418).


http://gerrit.cloudera.org:8080/#/c/12138/2/src/kudu/integration-tests/location_assignment-itest.cc
File src/kudu/integration-tests/location_assignment-itest.cc:

PS2: 
The diff here is weird; it's just include changes. I thought there was also a test change?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 04 Jan 2019 20:12:35 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................

Support location awareness in READ_CLOSEST for the C++ client

Previously, in READ_CLOSEST, the C++ client would choose to read from a
local tablet server if possible and otherwise pick a random replica.
This changes and enhances READ_CLOSEST mode to work as follows:

1. If there is a local server, use it. If there are multiple local
   servers, choose one at random.
2. If there is a server in the same location as the client, use it. If
   there are multiple servers in the same location, choose one at
   random.
3. Otherwise, choose a server at random.

This is not a breaking change, as in the absence of locations the
behavior is consistent with what was documented before, which was only
that a local server would be chosen if possible, else a random server.

Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
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/integration-tests/location_assignment-itest.cc
7 files changed, 146 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@422
PS7, Line 422: rand
> I opened a TODO item to verify that: https://issues.apache.org/jira/browse/
I meant 'Let that be out of the scope for this changelist.'



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 18:17:23 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 9: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12138/9/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12138/9/src/kudu/client/client-internal.cc@407
PS9, Line 407:       // TODO(wdberkeley): Eventually, the client might use the hierarchical
Can you add this TODO to your Java patches too?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 16:32:30 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 9: Code-Review+2

Nice catch regarding the flakiness due a rejected scan.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 17:22:05 +0000
Gerrit-HasComments: No

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 7: Verified+1

Unrelated flake.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 00:41:29 +0000
Gerrit-HasComments: No

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 8:

A follow-up's precommit uncovered flakiness in location_awareness-itest. Further investigation found the test was about 1% flaky in ASAN. See the new comment in the test for more info. Got 1000/1000 pass in ASAN mode with dist-test after the fix.


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@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: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 09:10:00 +0000
Gerrit-HasComments: No

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 8: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@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: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 19:05:00 +0000
Gerrit-HasComments: No

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 5:

(4 comments)

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

http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc@51
PS3, Line 51: 
> warning: using decl 'KuduClientBuilder' is unused [misc-unused-using-decls]
Done


http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc@78
PS3, Line 78:   ThreadSafeRandom rng(SeedRandom());
            :   int client_loc_idx = rng.Uniform(FLAGS_num_tablet_servers);
            :   for (int i = 0; i < FLAGS_num_tablet_servers; i++) {
            :     auto location = Substitute("/L$0", i);
            :     EmplaceOrDie(&info, Substitute("/L$0", i), i == client_loc_idx ? 2 : 1);
            :   }
> Nit: combine:
Done


http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc@90
PS3, Line 90:   // Find the tablet server that will be colocated with the client.
> Is this really necessary here? Won't BuildAndStart() fail in some way?
Done


http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc@109
PS3, Line 109:   vector<string> rows;
             :   ASSERT_OK(ScanToStrings(&scanner, &rows));
             :   ASSERT_TRUE(rows.empty());
             : 
> Why do we need a new KuduClient and KuduTable? Can't we reuse what's in Tab
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 07 Jan 2019 18:33:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 8: Code-Review+2


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@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: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 06:52:10 +0000
Gerrit-HasComments: No

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 7:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@406
PS7, Line 406: random
nit: does it make sense to add a TODO for the future to make use of the location string's hierarchical structure?


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@408
PS7, Line 408:       vector<RemoteTabletServer*> local;
             :       vector<RemoteTabletServer*> same_location;
nit: call reserve(filtered.size()) for local and same_location?


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@414
PS7, Line 414:         const string replica_location = rts->location();
nit: maybe, gate computing/calling this based on !client_location.empty()?


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@422
PS7, Line 422: rand
Do we init random generator in the client library?  I guess my question is whether we want results or this random choice to be random from one run to another.


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc
File src/kudu/integration-tests/location_assignment-itest.cc:

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc@79
PS7, Line 79: ThreadSafeRandom rng(SeedRandom());
nit: it seems TabletServerIntegrationTestBase has ThreadSafeRandom as aggregated member 'random_'; maybe use it here instead of creating a new one?


http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/integration-tests/location_assignment-itest.cc@125
PS7, Line 125:     // When running on Linux, the client and tablet servers each have their own
             :     // IP in the local address space, so no tablet server will be considered
             :     // local to the client. If there is a tablet server in the same location as
             :     // the client, it will be the only tablet server scanned. Otherwise, some
             :     // random tablet server will be scanned.
nit: move this under the #if defined block below?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 06:39:54 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12138/7/src/kudu/client/client-internal.cc@422
PS7, Line 422: rand
> Do we init random generator in the client library?  I guess my question is 
I opened a TODO item to verify that: https://issues.apache.org/jira/browse/KUDU-2657

I think we can about this to be out of the scope for this changelist.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 08 Jan 2019 18:13:24 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 9: Code-Review+1


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@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: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 09:33:16 +0000
Gerrit-HasComments: No

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 3:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc@78
PS3, Line 78:     auto location = Substitute("/L$0", i);
            :     if (i == client_loc_idx) {
            :       EmplaceOrDie(&info, std::move(location), 2);
            :     } else {
            :       EmplaceOrDie(&info, std::move(location), 1);
            :     }
Nit: combine:

  EmplaceOrDie(&info, Substitute("/L$0", i), i == client_loc_idx ? 2 : 1);


http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc@90
PS3, Line 90:   NO_FATALS(cluster_->AssertNoCrashes());
Is this really necessary here? Won't BuildAndStart() fail in some way?


http://gerrit.cloudera.org:8080/#/c/12138/3/src/kudu/integration-tests/location_assignment-itest.cc@109
PS3, Line 109:   client::sp::shared_ptr<KuduClient> client;
             :   NO_FATALS(CreateClient(&client));
             :   client::sp::shared_ptr<KuduTable> table;
             :   ASSERT_OK(client->OpenTable(kTableId, &table));
Why do we need a new KuduClient and KuduTable? Can't we reuse what's in TabletServerIntegrationTestBase?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 04 Jan 2019 22:15:18 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12138/9/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:

http://gerrit.cloudera.org:8080/#/c/12138/9/src/kudu/client/client-internal.cc@407
PS9, Line 407:     if (addr.IsWildcard()) continue;
> Can you add this TODO to your Java patches too?
Done



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 09 Jan 2019 19:35:35 +0000
Gerrit-HasComments: Yes

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, 

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

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

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................

Support location awareness in READ_CLOSEST for the C++ client

Previously, in READ_CLOSEST, the C++ client would choose to read from a
local tablet server if possible and otherwise pick a random replica.
This changes and enhances READ_CLOSEST mode to work as follows:

1. If there is a local server, use it. If there are multiple local
   servers, choose one at random.
2. If there is a server in the same location as the client, use it. If
   there are multiple servers in the same location, choose one at
   random.
3. Otherwise, choose a server at random.

This is not a breaking change, as in the absence of locations the
behavior is consistent with what was documented before, which was only
that a local server would be chosen if possible, else a random server.

Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
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/integration-tests/location_assignment-itest.cc
7 files changed, 147 insertions(+), 24 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Support location awareness in READ CLOSEST for the C++ client

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

Change subject: Support location awareness in READ_CLOSEST for the C++ client
......................................................................

Support location awareness in READ_CLOSEST for the C++ client

Previously, in READ_CLOSEST, the C++ client would choose to read from a
local tablet server if possible and otherwise pick a random replica.
This changes and enhances READ_CLOSEST mode to work as follows:

1. If there is a local server, use it. If there are multiple local
   servers, choose one at random.
2. If there is a server in the same location as the client, use it. If
   there are multiple servers in the same location, choose one at
   random.
3. Otherwise, choose a server at random.

This is not a breaking change, as in the absence of locations the
behavior is consistent with what was documented before, which was only
that a local server would be chosen if possible, else a random server.

Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Reviewed-on: http://gerrit.cloudera.org:8080/12138
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-internal.h
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/integration-tests/location_assignment-itest.cc
7 files changed, 156 insertions(+), 24 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, but someone else must approve
  Kudu Jenkins: Verified
  Grant Henke: Looks good to me, but someone else must approve
  Alexey Serbin: Looks good to me, approved

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2c6bcc7479c5cf2e17cb6e368ca89a1eb7f21713
Gerrit-Change-Number: 12138
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>