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/08/03 19:24:47 UTC
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11115
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
[location_awareness] Assign locations to registering tablet servers
This patch introduces a location field maintained by the master for
each tablet server. The master determines the value of this field
whenever a tablet server registers. It does this by using an
external command, specified with the flag --location_mapping_cmd,
to produce a location from the hostname or IP address of the tablet
server.
To help with ad hoc testing, I also added the location field to the
/tablet-servers web page, and fixed a small oversight where the
table of tablet servers wasn't sorted, so its order changed depending
on the order tablet servers first registered in.
I also altered the DATA_FILES CMake function so that data files copied
to the build directory or to slaves by dist-test are executable as well
as readable. This was necessary for the new TSDescriptor test which
tests location assignment.
Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
---
M CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master_path_handlers.cc
A src/kudu/master/testdata/first_argument.sh
A src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M www/tablet-servers.mustache
8 files changed, 313 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/11115/1
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11115/7/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/11115/7/src/kudu/master/master_path_handlers.cc@101
PS7, Line 101: left->permanent_uuid() < right->permanent_uuid()
> Is this supposed to work even for nullptrs? Or it should SIGSEGV and that'
It should be impossible for that to happen. I will enforce it with a DCHECK.
http://gerrit.cloudera.org:8080/#/c/11115/8/src/kudu/master/ts_descriptor-test.cc
File src/kudu/master/ts_descriptor-test.cc:
PS8:
> Do you think it's worth adding an integration test to verify that a tablet
Done
http://gerrit.cloudera.org:8080/#/c/11115/7/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/7/src/kudu/master/ts_descriptor.cc@231
PS7, Line 231: string location;
: TRACE("Assigning location");
: Status s = GetLocationFromLocationMappingCmd(location_ma
> Should we set some limit on how long this is going to take? For the extrem
In order to implement a timeout, we ought to run the location assignment in the background. This would introduce a lot more complexity into the process, especially if we respond early and "provisionally register" the tablet server (implying a need to possibly roll back a registration). But I think the location mapping command shouldn't be doing anything that has a realistic possibility of taking a while, like DNS or network requests. As long as users have tools to see if registration takes a long time, I think that's sufficient, because the solution to "sometimes by location assignment takes 2 minutes" is "you are doing it wrong- change the script".
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 04 Sep 2018 20:48:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11115/9/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/9/src/kudu/master/ts_descriptor.cc@62
PS9, Line 62:
> nit: it seems this flag is now run-time. Maybe, it's worth reflecting that
To me, a runtime flag means one it's ok to change at runtime and that we are blessing as ok to change. I don't want users to change the flag at runtime, and in fact I think we'd have a data race between changing the flag and its value being used to assign a location.
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 04 Sep 2018 21:50:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@224
PS3, Line 224: ResolveSockaddr
> 1) and 2) are valid points, but they don't apply strictly to this patch. Th
Last time I checked, tablet servers can be run with multiple RPC addresses, and those are recorded in a tablet server registration. What do you mean 'don't support multiple RPC addresses for a node'?
Probably, that's outside of the scope of this patch, but my concern is not about batch registration process, rather what happens if tablet server 'moves' from location to another because of multiple DNS records. There might be an inconsistencies due to that (e.g., the rebalancer tool might be confused with that). Probably, we can have a separate tool which would run some diagnostics to figure out whether the location assignment is 'flaky'.
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 06 Aug 2018 18:46:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/master_path_handlers.cc@117
PS4, Line 117: shared_ptr<TSDescriptor>
> nit: go a bit further and use auto here? Feel free to ignore, though.
Done
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor-test.cc
File src/kudu/master/ts_descriptor-test.cc:
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor-test.cc@86
PS4, Line 86: const string location = "/foo-bar0/BAAZ.9-quux";
> nit: maybe, add underscore as well
Done
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor-test.cc@86
PS4, Line 86: const string location = "/foo-bar0/BAAZ.9-quux";
> nit: maybe, add underscore as well
Done
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor-test.cc@103
PS4, Line 103: "/foo$", // Contains the illegal character '$'.
> nit: maybe, add "\" \"" just in case
That's the first test case in this vector.
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor.cc@241
PS4, Line 241: LOG(ERROR) << Substitute(
Think we should do something more than logging the failed registration attempt? Right now if location assignment isn't working the master logs about it over and over as the tablet server retries registration indefinitely.
At least, the verbosity should be reduced since registration will be attempted once per second.
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 09 Aug 2018 17:36:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Fengling Wang, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11115
to look at the new patch set (#5).
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
[location_awareness] Assign locations to registering tablet servers
This patch introduces a location field maintained by the master for
each tablet server. The master determines the value of this field
whenever a tablet server registers. It does this by using an
external command, specified with the flag --location_mapping_cmd,
to produce a location from the hostname or IP address of the tablet
server.
To help with ad hoc testing, I also added the location field to the
/tablet-servers web page, and fixed a small oversight where the
table of tablet servers wasn't sorted, so its order changed depending
on the order tablet servers first registered in.
I also altered the DATA_FILES CMake function so that data files copied
to the build directory or to slaves by dist-test are executable as well
as readable. This was necessary for the new TSDescriptor test which
tests location assignment.
Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
---
M CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master_path_handlers.cc
A src/kudu/master/testdata/first_argument.sh
A src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M www/tablet-servers.mustache
8 files changed, 300 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/11115/5
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 7: Verified+1
Unrelated concurrent rebalancers test flake.
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 04 Sep 2018 17:33:36 +0000
Gerrit-HasComments: No
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11115
to look at the new patch set (#2).
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
[location_awareness] Assign locations to registering tablet servers
This patch introduces a location field maintained by the master for
each tablet server. The master determines the value of this field
whenever a tablet server registers. It does this by using an
external command, specified with the flag --location_mapping_cmd,
to produce a location from the hostname or IP address of the tablet
server.
To help with ad hoc testing, I also added the location field to the
/tablet-servers web page, and fixed a small oversight where the
table of tablet servers wasn't sorted, so its order changed depending
on the order tablet servers first registered in.
I also altered the DATA_FILES CMake function so that data files copied
to the build directory or to slaves by dist-test are executable as well
as readable. This was necessary for the new TSDescriptor test which
tests location assignment.
Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
---
M CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master_path_handlers.cc
A src/kudu/master/testdata/first_argument.sh
A src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M www/tablet-servers.mustache
8 files changed, 318 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/11115/2
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Fengling Wang, Alexey Serbin,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11115
to look at the new patch set (#8).
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
[location_awareness] Assign locations to registering tablet servers
This patch introduces a location field maintained by the master for
each tablet server. The master determines the value of this field
whenever a tablet server registers. It does this by using an
external command, specified with the flag --location_mapping_cmd,
to produce a location from the hostname or IP address of the tablet
server.
To help with ad hoc testing, I also added the location field to the
/tablet-servers web page, and fixed a small oversight where the
table of tablet servers wasn't sorted, so its order changed depending
on the order tablet servers first registered in.
I also altered the DATA_FILES CMake function so that data files copied
to the build directory or to slaves by dist-test are executable as well
as readable. This was necessary for the new TSDescriptor test which
tests location assignment.
Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
---
M CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master_path_handlers.cc
A src/kudu/master/testdata/first_argument.sh
A src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M www/tablet-servers.mustache
8 files changed, 304 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/11115/8
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor.cc@241
PS4, Line 241: "Unable to assign loc
> Think we should do something more than logging the failed registration atte
Yeah, I think it's a good idea to at least reduce frequency of log messages in that case.
What can we do in addition to that? Maybe, add a metric to track number of 'relatively' new registration failures? Having such sort of metrics will allow to build a notifications to track registration failures.
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 09 Aug 2018 23:51:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11115/9/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/9/src/kudu/master/ts_descriptor.cc@62
PS9, Line 62:
> To me, a runtime flag means one it's ok to change at runtime and that we ar
As I can see, de facto all run-time flags in Kudu have that race condition, but it's not a bit deal. But we could use thread-safe gflags::GetCommandLineOption(), if needed, since GenericServiceImpl::SetFlag() uses gflags::SetCommandLineOption() which is thread-safe: https://github.com/gflags/gflags/blob/master/src/gflags.h.in#L206
However, if you think it's not worth it, that's OK with me.
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 05 Sep 2018 00:54:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11115/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11115/3//COMMIT_MSG@21
PS3, Line 21: I also altered the DATA_FILES CMake function so that data files copied
: to the build directory or to slaves by dist-test are executable as well
: as readable. This was necessary for the new TSDescriptor test which
: tests location assignment.
> nit: does it make sense to separate this into its own patch?
To me, not really. It's an update to a comment and adding two words to one line. A separate patch will just be that tiny change and a commit message saying "I need to do this for another patch".
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@176
PS3, Line 176: CHECK_EQ(instance.permanent_uuid(), permanent_uuid_);
:
: // TODO(KUDU-418): we don't currently support changing RPC addresses since the
: // host/port is stored persistently in each tablet's metadata.
: if (registration_ &&
: !HostPortPBsEqual(registration_->rpc_addresses(), registration.rpc_addresses())) {
: string msg = strings::Substitute(
: "Tablet server $0 is attempting to re-register with a different host/port. "
: "This is not currently supported. Old: {$1} New: {$2}",
: instance.permanent_uuid(),
: SecureShortDebugString(*registration_),
: SecureShortDebugString(registration));
: LOG(ERROR) << msg;
: return Status::InvalidArgument(msg);
: }
:
: if (registration.rpc_addresses().empty()) {
: return Status::InvalidArgument(
: "invalid registration: must have at least one RPC address",
: SecureShortDebugString(registration));
: }
:
: if (instance.instance_seqno() < latest_seqno_) {
: return Status::AlreadyPresent(
: strings::Substitute("Cannot register with sequence number $0:"
: " Already have a registration from sequence number $1",
: instance.instance_seqno(),
: latest_seqno_));
: } else if (instance.instance_seqno() == latest_seqno_) {
: // It's possible that the TS registered, but our response back to it
: // got lost, so it's trying to register again with the same sequence
: // number. That's fine.
: LOG(INFO) << "Processing retry of TS registration from " << SecureShortDebugString(instance);
: }
:
: latest_seqno_ = instance.instance_seqno();
: registration_.reset(new ServerRegistrationPB(registration));
: ts_admin_proxy_.reset();
: consensus_proxy_.reset();
> nit: maybe, extract this into a method? RegisterUnlocked?
Done
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@224
PS3, Line 224: ResolveSockaddr
> I have some concerns about using thing method.
1) and 2) are valid points, but they don't apply strictly to this patch. They apply to Kudu in general. We don't support multiple RPC addresses for a node. I think doing so here would be surprising if the support is lacking elsewhere. Why assign a location based on an address that Kudu can never use?
We spoke about the second issue a bit. The script should accept IP or hostname, as the address may resolve into either. For this first iteration, it doesn't seem necessary to resolve multiple ip/hostnames into locations. If we were to do that, we ought to use it, and to use it we'd need to devise some kind of batch tablet server registration process or a batch location assignment process for clients.
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@233
PS3, Line 233: location
> nit: std::move(location) ?
Done
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 06 Aug 2018 18:23:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 6: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 24 Aug 2018 16:42:45 +0000
Gerrit-HasComments: No
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 4:
(5 comments)
LGTM, just a few nits
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/master_path_handlers.cc@117
PS4, Line 117: shared_ptr<TSDescriptor>
nit: go a bit further and use auto here? Feel free to ignore, though.
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor-test.cc
File src/kudu/master/ts_descriptor-test.cc:
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor-test.cc@86
PS4, Line 86: const string location = "/foo-bar0/BAAZ.9-quux";
nit: maybe, add underscore as well
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor-test.cc@86
PS4, Line 86: const string location = "/foo-bar0/BAAZ.9-quux";
nit: maybe, add underscore as well
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor-test.cc@103
PS4, Line 103: "/foo$", // Contains the illegal character '$'.
nit: maybe, add "\" \"" just in case
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/4/src/kudu/master/ts_descriptor.cc@113
PS4, Line 113: StripTrailingWhitespace
nit: maybe, use StripWhiteSpace() to trim both leading the trailing whitespaces?
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 08 Aug 2018 05:37:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
[location_awareness] Assign locations to registering tablet servers
This patch introduces a location field maintained by the master for
each tablet server. The master determines the value of this field
whenever a tablet server registers. It does this by using an
external command, specified with the flag --location_mapping_cmd,
to produce a location from the hostname or IP address of the tablet
server.
To help with ad hoc testing, I also added the location field to the
/tablet-servers web page, and fixed a small oversight where the
table of tablet servers wasn't sorted, so its order changed depending
on the order tablet servers first registered in.
I also altered the DATA_FILES CMake function so that data files copied
to the build directory or to slaves by dist-test are executable as well
as readable. This was necessary for the new TSDescriptor test which
tests location assignment.
Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Reviewed-on: http://gerrit.cloudera.org:8080/11115
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
M src/kudu/master/master_path_handlers.cc
A src/kudu/master/testdata/first_argument.sh
A src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M www/tablet-servers.mustache
9 files changed, 342 insertions(+), 12 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 10
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 3:
(4 comments)
Took a quick look and decided to post a couple of questions before reviewing the rest of the patch.
http://gerrit.cloudera.org:8080/#/c/11115/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11115/3//COMMIT_MSG@21
PS3, Line 21: I also altered the DATA_FILES CMake function so that data files copied
: to the build directory or to slaves by dist-test are executable as well
: as readable. This was necessary for the new TSDescriptor test which
: tests location assignment.
nit: does it make sense to separate this into its own patch?
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@176
PS3, Line 176: CHECK_EQ(instance.permanent_uuid(), permanent_uuid_);
:
: // TODO(KUDU-418): we don't currently support changing RPC addresses since the
: // host/port is stored persistently in each tablet's metadata.
: if (registration_ &&
: !HostPortPBsEqual(registration_->rpc_addresses(), registration.rpc_addresses())) {
: string msg = strings::Substitute(
: "Tablet server $0 is attempting to re-register with a different host/port. "
: "This is not currently supported. Old: {$1} New: {$2}",
: instance.permanent_uuid(),
: SecureShortDebugString(*registration_),
: SecureShortDebugString(registration));
: LOG(ERROR) << msg;
: return Status::InvalidArgument(msg);
: }
:
: if (registration.rpc_addresses().empty()) {
: return Status::InvalidArgument(
: "invalid registration: must have at least one RPC address",
: SecureShortDebugString(registration));
: }
:
: if (instance.instance_seqno() < latest_seqno_) {
: return Status::AlreadyPresent(
: strings::Substitute("Cannot register with sequence number $0:"
: " Already have a registration from sequence number $1",
: instance.instance_seqno(),
: latest_seqno_));
: } else if (instance.instance_seqno() == latest_seqno_) {
: // It's possible that the TS registered, but our response back to it
: // got lost, so it's trying to register again with the same sequence
: // number. That's fine.
: LOG(INFO) << "Processing retry of TS registration from " << SecureShortDebugString(instance);
: }
:
: latest_seqno_ = instance.instance_seqno();
: registration_.reset(new ServerRegistrationPB(registration));
: ts_admin_proxy_.reset();
: consensus_proxy_.reset();
nit: maybe, extract this into a method? RegisterUnlocked?
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@224
PS3, Line 224: ResolveSockaddr
I have some concerns about using thing method.
1) this method resolves just one of tserver's PRC end-points
2) if the end-point has multiple DNS names, it outputs just one
From that perspective, could it be the case when a tablet server
a) doesn't get the desired/expected location
b) moves from one location to another just because of DNS round-robins multiple names
?
Also, just another point to think about the design a bit: should the location script be able to take
a) just IP address
b) multiple IP addresses
c) multiple DNS names
?
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@233
PS3, Line 233: location
nit: std::move(location) ?
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 06 Aug 2018 17:56:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 9: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11115/7/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/7/src/kudu/master/ts_descriptor.cc@231
PS7, Line 231: string location;
: TRACE("Assigning location");
: Status s = GetLocationFromLocationMappingCmd(location_ma
> In order to implement a timeout, we ought to run the location assignment in
Yes, I think it's not worth complicating this unless there is an urgency in doing so.
Maybe, it's worth adding a metric to count failed runs of the location assignment command, but otherwise I think it's possible to track that in the master's log.
http://gerrit.cloudera.org:8080/#/c/11115/9/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/9/src/kudu/master/ts_descriptor.cc@62
PS9, Line 62:
nit: it seems this flag is now run-time. Maybe, it's worth reflecting that information in here.
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 04 Sep 2018 21:34:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11115/8/src/kudu/master/ts_descriptor-test.cc
File src/kudu/master/ts_descriptor-test.cc:
PS8:
Do you think it's worth adding an integration test to verify that a tablet server isn't not visible to the clients if its registration script fails?
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 04 Sep 2018 20:11:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/3/src/kudu/master/ts_descriptor.cc@224
PS3, Line 224: ResolveSockaddr
> Last time I checked, tablet servers can be run with multiple RPC addresses,
I think we can avoid using DNS if we are willing to accept using the host from the first listed rpc address for the tablet server (and we could document this is how locations are resolved).
Location assignment can only change as often as the TS reregisters, which is only as often as the TS restarts.
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 08 Aug 2018 00:02:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Fengling Wang, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11115
to look at the new patch set (#9).
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
[location_awareness] Assign locations to registering tablet servers
This patch introduces a location field maintained by the master for
each tablet server. The master determines the value of this field
whenever a tablet server registers. It does this by using an
external command, specified with the flag --location_mapping_cmd,
to produce a location from the hostname or IP address of the tablet
server.
To help with ad hoc testing, I also added the location field to the
/tablet-servers web page, and fixed a small oversight where the
table of tablet servers wasn't sorted, so its order changed depending
on the order tablet servers first registered in.
I also altered the DATA_FILES CMake function so that data files copied
to the build directory or to slaves by dist-test are executable as well
as readable. This was necessary for the new TSDescriptor test which
tests location assignment.
Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
---
M CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master-test.cc
M src/kudu/master/master_path_handlers.cc
A src/kudu/master/testdata/first_argument.sh
A src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M www/tablet-servers.mustache
9 files changed, 342 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/11115/9
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 9
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11115/7/src/kudu/master/master_path_handlers.cc
File src/kudu/master/master_path_handlers.cc:
http://gerrit.cloudera.org:8080/#/c/11115/7/src/kudu/master/master_path_handlers.cc@101
PS7, Line 101: left->permanent_uuid() < right->permanent_uuid()
Is this supposed to work even for nullptrs? Or it should SIGSEGV and that's the expected behavior?
http://gerrit.cloudera.org:8080/#/c/11115/7/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11115/7/src/kudu/master/ts_descriptor.cc@231
PS7, Line 231: string location;
: TRACE("Assigning location");
: Status s = GetLocationFromLocationMappingCmd(location_ma
Should we set some limit on how long this is going to take? For the extreme case, imagine this takes 3 minutes for the script to return. Is it OK to wait for a long time here in the context of MasterServiceImpl::TSHeartbeat()?
Another thought: how is it important to make sure the location is assigned before sending back the OK response? From the code above I see the entry will be updated anyways, but only location will not be set if the location assignment script returns an error. Maybe, we should run the location assignment in the background?
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 04 Sep 2018 18:51:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11115 )
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
Patch Set 6: Verified+1
Unrelated flake in KuduTsCliTest.TestDumpTablet
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 24 Aug 2018 16:42:35 +0000
Gerrit-HasComments: No
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11115
to look at the new patch set (#3).
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
[location_awareness] Assign locations to registering tablet servers
This patch introduces a location field maintained by the master for
each tablet server. The master determines the value of this field
whenever a tablet server registers. It does this by using an
external command, specified with the flag --location_mapping_cmd,
to produce a location from the hostname or IP address of the tablet
server.
To help with ad hoc testing, I also added the location field to the
/tablet-servers web page, and fixed a small oversight where the
table of tablet servers wasn't sorted, so its order changed depending
on the order tablet servers first registered in.
I also altered the DATA_FILES CMake function so that data files copied
to the build directory or to slaves by dist-test are executable as well
as readable. This was necessary for the new TSDescriptor test which
tests location assignment.
Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
---
M CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master_path_handlers.cc
A src/kudu/master/testdata/first_argument.sh
A src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M www/tablet-servers.mustache
8 files changed, 319 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/11115/3
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [location awareness] Assign locations to registering tablet servers
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Fengling Wang, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11115
to look at the new patch set (#4).
Change subject: [location_awareness] Assign locations to registering tablet servers
......................................................................
[location_awareness] Assign locations to registering tablet servers
This patch introduces a location field maintained by the master for
each tablet server. The master determines the value of this field
whenever a tablet server registers. It does this by using an
external command, specified with the flag --location_mapping_cmd,
to produce a location from the hostname or IP address of the tablet
server.
To help with ad hoc testing, I also added the location field to the
/tablet-servers web page, and fixed a small oversight where the
table of tablet servers wasn't sorted, so its order changed depending
on the order tablet servers first registered in.
I also altered the DATA_FILES CMake function so that data files copied
to the build directory or to slaves by dist-test are executable as well
as readable. This was necessary for the new TSDescriptor test which
tests location assignment.
Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
---
M CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/master/master_path_handlers.cc
A src/kudu/master/testdata/first_argument.sh
A src/kudu/master/ts_descriptor-test.cc
M src/kudu/master/ts_descriptor.cc
M src/kudu/master/ts_descriptor.h
M www/tablet-servers.mustache
8 files changed, 295 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/11115/4
--
To view, visit http://gerrit.cloudera.org:8080/11115
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5eb98823ab7b3b8141b8630196c29c1ebf8e6878
Gerrit-Change-Number: 11115
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Fengling Wang <fw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>