[kudu-CR] [location awareness] Add 'location' column in tserver list
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. [location_awareness] Add 'location' column in tserver list Command: kudu tserver list -columns=uuid,location Example result: uuid | location --+-- 1259764cdc5f489984900d49b545802f | loc0 14446895a8bf47cd92e73836de623ffb | 9d7a11e19b324f62b2e6d074f6003ca4 | loc1 This command will list the location of each tserver. If the location of the tserver has not been set, '' will be displayed. Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Reviewed-on: http://gerrit.cloudera.org:8080/11313 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin Reviewed-by: Will Berkeley --- M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/first_argument.sh M src/kudu/tools/tool_action_tserver.cc 6 files changed, 61 insertions(+), 1 deletion(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, but someone else must approve Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 7 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Add 'location' column in tserver list
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. Patch Set 6: NB we can't merge this until the assignment patch is merged. -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 6 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 04 Sep 2018 20:55:27 + Gerrit-HasComments: No
[kudu-CR] [location awareness] Add 'location' column in tserver list
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 6 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 04 Sep 2018 20:53:51 + Gerrit-HasComments: No
[kudu-CR] [location awareness] Add 'location' column in tserver list
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 6 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 04 Sep 2018 18:24:07 + Gerrit-HasComments: No
[kudu-CR] [location awareness] Add 'location' column in tserver list
Fengling Wang has posted comments on this change. (
http://gerrit.cloudera.org:8080/11313 )
Change subject: [location_awareness] Add 'location' column in tserver list
..
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc@2038
PS5, Line 2038: ListL
> Is this a typo? 'List' or 'Listing'?
Opps sorry. Done
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc@2043
PS5, Line 2043:
: Substitute("--location_mapping_cmd=$0 $1", kLoc
> nit: why not just
Done
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc@2057
PS5, Line 2057: ListL
> ditto: is this a typo?
Done
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/tool_action_tserver.cc@144
PS5, Line 144: std
> nit: add std::move:
Done
--
To view, visit http://gerrit.cloudera.org:8080/11313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8
Gerrit-Change-Number: 11313
Gerrit-PatchSet: 6
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Fengling Wang
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Fri, 31 Aug 2018 20:47:50 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Greg Solovyev, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11313 to look at the new patch set (#6). Change subject: [location_awareness] Add 'location' column in tserver list .. [location_awareness] Add 'location' column in tserver list Command: kudu tserver list -columns=uuid,location Example result: uuid | location --+-- 1259764cdc5f489984900d49b545802f | loc0 14446895a8bf47cd92e73836de623ffb | 9d7a11e19b324f62b2e6d074f6003ca4 | loc1 This command will list the location of each tserver. If the location of the tserver has not been set, '' will be displayed. Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 --- M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/first_argument.sh M src/kudu/tools/tool_action_tserver.cc 6 files changed, 61 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/11313/6 -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 6 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Add 'location' column in tserver list
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11313 )
Change subject: [location_awareness] Add 'location' column in tserver list
..
Patch Set 5:
(4 comments)
overall looks good, just a few nits
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc@2038
PS5, Line 2038: Listi
Is this a typo? 'List' or 'Listing'?
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc@2043
PS5, Line 2043: "--location_mapping_cmd=" +
: Substitute("$0 $1", kLocationCmdPath, location)
nit: why not just
Substitute("--location_mapping_cmd=$0 $1", kLocationCmdPath, location)
?
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/kudu-tool-test.cc@2057
PS5, Line 2057: Listi
ditto: is this a typo?
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/11313/5/src/kudu/tools/tool_action_tserver.cc@144
PS5, Line 144: loc
nit: add std::move:
std::move(loc)
--
To view, visit http://gerrit.cloudera.org:8080/11313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8
Gerrit-Change-Number: 11313
Gerrit-PatchSet: 5
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Fengling Wang
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Fri, 31 Aug 2018 05:48:18 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Greg Solovyev, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11313 to look at the new patch set (#5). Change subject: [location_awareness] Add 'location' column in tserver list .. [location_awareness] Add 'location' column in tserver list Command: kudu tserver list -columns=uuid,location Example result: uuid | location --+-- 1259764cdc5f489984900d49b545802f | loc0 14446895a8bf47cd92e73836de623ffb | 9d7a11e19b324f62b2e6d074f6003ca4 | loc1 This command will list the location of each tserver. If the location of the tserver has not been set, '' will be displayed. Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 --- M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/first_argument.sh M src/kudu/tools/tool_action_tserver.cc 6 files changed, 61 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/11313/5 -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 5 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Add 'location' column in tserver list
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. Patch Set 4: > Uploaded patch set 4. It seems lint is not yet happy, please take a look at: http://jenkins.kudu.apache.org/job/kudu-gerrit/14720/BUILD_TYPE=LINT/artifact/build/latest/test-logs/lint.log You can ran the link checker against your local workspace using 'ilint' target: make ilint in the build directory -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 4 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 28 Aug 2018 23:01:55 + Gerrit-HasComments: No
[kudu-CR] [location awareness] Add 'location' column in tserver list
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11313/2/src/kudu/tools/tool_action_tserver.cc File src/kudu/tools/tool_action_tserver.cc: http://gerrit.cloudera.org:8080/#/c/11313/2/src/kudu/tools/tool_action_tserver.cc@141 PS2, Line 141: location > Good catch but could you tell me why would it be cleaner? Thought we might This is just my subjective opinion. Using constants for strings that are used more than once has two advantages. One - compiler will catch typos and spelling errors. Two - if the name of the column changes for some reason (even if this is highly unlikely), we will avoid the risk of forgetting to change the string in any of places where it is used. -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 4 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 28 Aug 2018 22:49:41 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11313/3/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/11313/3/src/kudu/master/master_service.cc@419 PS3, Line 419: if (desc->location( > I think it's better to not set this field at all if location in the descrip Done -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 4 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 28 Aug 2018 21:21:54 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Greg Solovyev, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11313 to look at the new patch set (#4). Change subject: [location_awareness] Add 'location' column in tserver list .. [location_awareness] Add 'location' column in tserver list Command: kudu tserver list -columns=uuid,location Example result: uuid | location --+-- 1259764cdc5f489984900d49b545802f | loc0 14446895a8bf47cd92e73836de623ffb | 9d7a11e19b324f62b2e6d074f6003ca4 | loc1 This command will list the location of each tserver. If the location of the tserver has not been set, '' will be displayed. Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 --- M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/first_argument.sh M src/kudu/tools/tool_action_tserver.cc 6 files changed, 61 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/11313/4 -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 4 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Add 'location' column in tserver list
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11313 )
Change subject: [location_awareness] Add 'location' column in tserver list
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11313/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11313/3//COMMIT_MSG@20
PS3, Line 20: not assgined
http://gerrit.cloudera.org:8080/#/c/11313/3/src/kudu/master/master_service.cc
File src/kudu/master/master_service.cc:
http://gerrit.cloudera.org:8080/#/c/11313/3/src/kudu/master/master_service.cc@419
PS3, Line 419: entry->set_location
I think it's better to not set this field at all if location in the descriptor
is not set:
if (desc->location()) {
entry->set_location(...);
}
--
To view, visit http://gerrit.cloudera.org:8080/11313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8
Gerrit-Change-Number: 11313
Gerrit-PatchSet: 3
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Fengling Wang
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 28 Aug 2018 21:07:28 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Fengling Wang has posted comments on this change. (
http://gerrit.cloudera.org:8080/11313 )
Change subject: [location_awareness] Add 'location' column in tserver list
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11313/2/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/11313/2/src/kudu/tools/tool_action_tserver.cc@141
PS2, Line 141: location
> I noticed that we reuse these column name strings ("version", "http-address
Good catch but could you tell me why would it be cleaner? Thought we might need
extra lines declaring the constant strings.? And since there are just two files
involved, I wonder if it's really necessary to make the column name strings
shared?
--
To view, visit http://gerrit.cloudera.org:8080/11313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8
Gerrit-Change-Number: 11313
Gerrit-PatchSet: 2
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Fengling Wang
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 28 Aug 2018 21:04:46 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, Greg Solovyev, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11313 to look at the new patch set (#3). Change subject: [location_awareness] Add 'location' column in tserver list .. [location_awareness] Add 'location' column in tserver list Command: kudu tserver list -columns=uuid,location Example result: uuid | location --+-- 1259764cdc5f489984900d49b545802f | loc0 14446895a8bf47cd92e73836de623ffb | 9d7a11e19b324f62b2e6d074f6003ca4 | loc1 This command will list the location of each tserver. If the location of the tserver has not been set, 'not assgined' will be displayed. Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 --- M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/first_argument.sh M src/kudu/tools/tool_action_tserver.cc 6 files changed, 61 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/11313/3 -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 3 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Add 'location' column in tserver list
Greg Solovyev has posted comments on this change. (
http://gerrit.cloudera.org:8080/11313 )
Change subject: [location_awareness] Add 'location' column in tserver list
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11313/2/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/11313/2/src/kudu/tools/tool_action_tserver.cc@141
PS2, Line 141: location
I noticed that we reuse these column name strings ("version", "http-addresses",
"seqno") in two files (tool_action_tserver.cc and too_action_master.cc). Would
it be cleaner to have a shared string constant?
--
To view, visit http://gerrit.cloudera.org:8080/11313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8
Gerrit-Change-Number: 11313
Gerrit-PatchSet: 2
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Fengling Wang
Gerrit-Reviewer: Greg Solovyev
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Tue, 28 Aug 2018 18:55:10 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. Patch Set 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/11313/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11313/1//COMMIT_MSG@16 PS1, Line 16: > I'd prefer a value like . There's a couple of advantages to it: Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/common/wire_protocol.proto File src/kudu/common/wire_protocol.proto: http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/common/wire_protocol.proto@84 PS1, Line 84: > ServerRegistrationPB is also used for both master and tservers. Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/common/wire_protocol.proto@84 PS1, Line 84: > I think NodeInstancePB is about any kudu server, i.e. masters are also mean Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/master/ts_descriptor.cc File src/kudu/master/ts_descriptor.cc: http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/master/ts_descriptor.cc@310 PS1, Line 310: } : > I agree we should leave the field unset if there's no location. For one thi Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2038 PS1, Line 2038: _F (ToolTest, TestTserverL > nit: drop the parentheses. Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2038 PS1, Line 2038: _F (ToolTest, TestTserverL > Also, comments end in a period. Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2047 PS1, Line 2047: > Consider using ASSERT_STR_CONTAINS macro. Or the spacing is really importa Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2050 PS1, Line 2050: ring out; > nit: drop the parentheses. Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2050 PS1, Line 2050: ring out; > Ditto period. Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2054 PS1, Line 2054: ASSERT_STR_CONTAINS(out, cluster_->tablet_server(0)->uuid() + "," + location); > Why is it important to set FLAGS_location_mapping_cmd if it's an external c Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2058 PS1, Line 2058: _FATALS(StartExternalMiniCluster()); > The minicluster object itself should have methods to stop it, and then I th Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2066 PS1, Line 2066: S(out, c > I think using --columns=uuid,location with --format=csv and then testing AS Done http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/testdata/first_argument.sh File src/kudu/tools/testdata/first_argument.sh: PS1: > You'll need to add this as a data file in the CMakeLists. See my patch to a Done -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 2 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 27 Aug 2018 23:09:20 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Hello Will Berkeley, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11313 to look at the new patch set (#2). Change subject: [location_awareness] Add 'location' column in tserver list .. [location_awareness] Add 'location' column in tserver list Command: kudu tserver list -columns=uuid,location Example result: uuid | location --+-- 1259764cdc5f489984900d49b545802f | loc0 14446895a8bf47cd92e73836de623ffb | 9d7a11e19b324f62b2e6d074f6003ca4 | loc1 This command will list the location of each tserver. If the location of the tserver has not been set, 'not assgined' will be displayed. Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 --- M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/first_argument.sh M src/kudu/tools/tool_action_tserver.cc 6 files changed, 60 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/11313/2 -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 2 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Add 'location' column in tserver list
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2058 PS1, Line 2058: NO_FATALS(StartExternalMiniCluster(opts)); > Maybe I'm wrong. I feel like StartExternalMiniCluster(opts) is the only met The minicluster object itself should have methods to stop it, and then I think it can be restarted. Separate tests is fine though. http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2066 PS1, Line 2066: > I was thinking ASSERT_STR_CONTAINS would overlook cases where there's garba I think using --columns=uuid,location with --format=csv and then testing ASSERT_STR_CONTAINS for , should be pretty solid. -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 1 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 24 Aug 2018 20:15:18 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11313 ) Change subject: [location_awareness] Add 'location' column in tserver list .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2058 PS1, Line 2058: NO_FATALS(StartExternalMiniCluster(opts)); > Yeah, we need to either make this a separate test where the flag is set bef Maybe I'm wrong. I feel like StartExternalMiniCluster(opts) is the only method where we can reset the flags. But we can't have this method called twice in a test. So I'm thinking of making two separate tests: TestTserverListLocation, and TestTserverListLocationNone. Does that sound right? http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2066 PS1, Line 2066: > Is it important to have this spacing to be some specified length? If not, I was thinking ASSERT_STR_CONTAINS would overlook cases where there's garbage string or extra rows. But yeah maybe ASSERT_STR_CONTAINS should be good enough. -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 1 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Fri, 24 Aug 2018 20:02:32 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/11313 )
Change subject: [location_awareness] Add 'location' column in tserver list
..
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11313/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11313/1//COMMIT_MSG@16
PS1, Line 16: not assigned
I'd prefer a value like . There's a couple of advantages to it:
1. It contains a character that is invalid in a location string, so it's
unambiguously not a location.
2. It doesn't have spaces in it, which is sometimes helpful when someone uses
an alternate format and processes the output with scripts.
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/common/wire_protocol.proto
File src/kudu/common/wire_protocol.proto:
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/common/wire_protocol.proto@84
PS1, Line 84: The tablet server's
> I think NodeInstancePB is about any kudu server, i.e. masters are also mean
ServerRegistrationPB is also used for both master and tservers.
I think the right place for location information is in
ListTabletServersResponsePB::Entry, since that is a PB used to describe a
tablet server specifically.
message Entry {
required NodeInstancePB instance_id = 1;
optional ServerRegistrationPB registration = 2;
optional int32 millis_since_heartbeat = 3;
optional string location = 4;
}
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/master/ts_descriptor.cc@310
PS1, Line 310: instance_pb->set_location(location_ == boost::none ?
: "" : string(location_->begin(),
location_->end()));
> Why to set it to an empty string if it's possible to not set it all in Node
I agree we should leave the field unset if there's no location. For one thing,
the PB object will return an empty string for the location when the location
isn't set.
Also, just a nit, but I prefer boost::optional::get() to retrieve the value out
of an optional, to distinguish it from a pointer because it has value semantics.
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2038
PS1, Line 2038: // Location (not assigned)
Also, comments end in a period.
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2050
PS1, Line 2050: // Location (assigned)
Ditto period.
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2058
PS1, Line 2058: NO_FATALS(StartExternalMiniCluster(opts));
> ditto: the external minicluster has been started already at line 1984, no?
Yeah, we need to either make this a separate test where the flag is set before
the cluster is started, or the cluster needs to be restarted with new flags.
Calling StartExternalMiniCluster again doesn't work.
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/testdata/first_argument.sh
File src/kudu/tools/testdata/first_argument.sh:
PS1:
You'll need to add this as a data file in the CMakeLists. See my patch to add
location assignment for an example.
--
To view, visit http://gerrit.cloudera.org:8080/11313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8
Gerrit-Change-Number: 11313
Gerrit-PatchSet: 1
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Fri, 24 Aug 2018 18:20:03 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11313 )
Change subject: [location_awareness] Add 'location' column in tserver list
..
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/common/wire_protocol.proto
File src/kudu/common/wire_protocol.proto:
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/common/wire_protocol.proto@84
PS1, Line 84: The tablet server's
I think NodeInstancePB is about any kudu server, i.e. masters are also meant to
be represented by this structure. How locations are about to be used in case
of masters is another question, though.
BTW, I remember the original idea was to put location into
ServerRegistrationPB. Did something changed in that regard? I.e., why do you
think NodeInstancePB is a better place for that?
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/master/ts_descriptor.cc
File src/kudu/master/ts_descriptor.cc:
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/master/ts_descriptor.cc@310
PS1, Line 310: instance_pb->set_location(location_ == boost::none ?
: "" : string(location_->begin(),
location_->end()));
Why to set it to an empty string if it's possible to not set it all in
NodeInstancPB?
Also, boost::optional has operator* () that allows to de-refence the embedded
value.
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2038
PS1, Line 2038: // Location (not assigned)
nit: drop the parentheses.
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2039
PS1, Line 2039: NO_FATALS(StartExternalMiniCluster());
Why is it necessary to start it again when it's been already started at line
1984?
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2047
PS1, Line 2047: " location\n--\n not assigned"
Consider using ASSERT_STR_CONTAINS macro. Or the spacing is really important
here?
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2050
PS1, Line 2050: // Location (assigned)
nit: drop the parentheses.
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2054
PS1, Line 2054: FLAGS_location_mapping_cmd = Substitute("$0 $1",
kLocationCmdPath, location);
Why is it important to set FLAGS_location_mapping_cmd if it's an external
cluster anyway and it's being passed the --location_mapping_cmd flag?
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2058
PS1, Line 2058: NO_FATALS(StartExternalMiniCluster(opts));
ditto: the external minicluster has been started already at line 1984, no?
http://gerrit.cloudera.org:8080/#/c/11313/1/src/kudu/tools/kudu-tool-test.cc@2066
PS1, Line 2066:
Is it important to have this spacing to be some specified length? If not,
consider using ASSERT_STR_CONTAINS macro instead.
--
To view, visit http://gerrit.cloudera.org:8080/11313
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8
Gerrit-Change-Number: 11313
Gerrit-PatchSet: 1
Gerrit-Owner: Fengling Wang
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Fri, 24 Aug 2018 17:25:41 +
Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add 'location' column in tserver list
Fengling Wang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11313 Change subject: [location_awareness] Add 'location' column in tserver list .. [location_awareness] Add 'location' column in tserver list Command: kudu tserver list -columns=uuid,location Example result: uuid | location --+-- 1259764cdc5f489984900d49b545802f | loc0 14446895a8bf47cd92e73836de623ffb | not assigned 9d7a11e19b324f62b2e6d074f6003ca4 | loc1 This command will list the location of each tserver. If the location of the tserver has not been set, 'not assgined' will be displayed. Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 --- M src/kudu/common/wire_protocol.proto M src/kudu/master/ts_descriptor.cc M src/kudu/tools/kudu-tool-test.cc A src/kudu/tools/testdata/first_argument.sh M src/kudu/tools/tool_action_tserver.cc 5 files changed, 62 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/11313/1 -- To view, visit http://gerrit.cloudera.org:8080/11313 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: If6c9dc8bd08b8d907111fac850fc6fd2c2b96fb8 Gerrit-Change-Number: 11313 Gerrit-PatchSet: 1 Gerrit-Owner: Fengling Wang
