[kudu-CR] [java] KUDU-3213: try at different server on TABLET NOT RUNNING
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/17124 )
Change subject: [java] KUDU-3213: try at different server on TABLET_NOT_RUNNING
..
Patch Set 4: Code-Review+1
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17124/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java
File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java:
http://gerrit.cloudera.org:8080/#/c/17124/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@96
PS4, Line 96: setRangePartitionColumns(Collections.singletonList("key"))
nit: would it make sense to use hash partitioning instead? Otherwise, how do
we know that the quiesce tablet server hosts the replica that contains the
necessary data? If it's so even with range-partitioned table, it would be
great if you could add a small comment explaining why it's so. Thanks!
http://gerrit.cloudera.org:8080/#/c/17124/4/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduScanner.java@120
PS4, Line 120: if the scan goes to the quiescing server
nit: how do we know it's so, indeed? Could it happen that the scanner always
hits only non-quested servers?
--
To view, visit http://gerrit.cloudera.org:8080/17124
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I38ac84a52676ff361fa1ba996665b338d1bbfba1
Gerrit-Change-Number: 17124
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 02 Mar 2021 02:52:22 +
Gerrit-HasComments: Yes
[kudu-CR] [client-test] fix TestFailedDnsResolution
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17142 ) Change subject: [client-test] fix TestFailedDnsResolution .. [client-test] fix TestFailedDnsResolution I noticed that ClientTest.TestFailedDnsResolution fails unexpectedly with the following error when running on macOS: src/kudu/client/client-test.cc:3205: Failure Value of: s.IsIOError() Actual: false Expected: true unexpected status: OK It turned out that the scenario didn't expect that (a) the results of DNS resolution are cached (b) a tablet server's address can be the same as master's (a) turned true with changelist 48467ccf4, and (b) is true in case of running test mini-cluster with other than UNIQUE_LOOPBACK bind mode: e.g., on macOS it's run in LOOPBACK mode. I updated the scenario to use a non-caching DNS resolver. I also increased the timeout for write operations because the scenario was failing from time to time in case TSAN builds. In addition, since timeouts in GetTableLocations RPC are reported two-fold due to the client's metacache activity, the test was failing rarely due to receiving other non-expected error message. I updated the list of expected error messages to add the missing case. With this patch, the scenario succeeds on macOS and runs more stable for Linux TSAN builds. Change-Id: I0493d992c43adb14ef02efae0a15dddc53301d7d Reviewed-on: http://gerrit.cloudera.org:8080/17142 Tested-by: Kudu Jenkins Reviewed-by: Bankim Bhavsar Reviewed-by: Andrew Wong --- M src/kudu/client/client-test.cc 1 file changed, 44 insertions(+), 29 deletions(-) Approvals: Kudu Jenkins: Verified Bankim Bhavsar: Looks good to me, but someone else must approve Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/17142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I0493d992c43adb14ef02efae0a15dddc53301d7d Gerrit-Change-Number: 17142 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [client-test] fix TestFailedDnsResolution
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17142 ) Change subject: [client-test] fix TestFailedDnsResolution .. Patch Set 1: > Thanks for addressing this! Confirmed it works on my Mac. Thank you for making sure it works on macOS as expected! Hopefully, at some point we can have all test passing on macOS again :) -- To view, visit http://gerrit.cloudera.org:8080/17142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0493d992c43adb14ef02efae0a15dddc53301d7d Gerrit-Change-Number: 17142 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 02 Mar 2021 02:37:52 + Gerrit-HasComments: No
[kudu-CR] [client-test] fix TestFailedDnsResolution
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17142 ) Change subject: [client-test] fix TestFailedDnsResolution .. Patch Set 1: Code-Review+2 Thanks for addressing this! Confirmed it works on my Mac. -- To view, visit http://gerrit.cloudera.org:8080/17142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0493d992c43adb14ef02efae0a15dddc53301d7d Gerrit-Change-Number: 17142 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 02 Mar 2021 02:27:56 + Gerrit-HasComments: No
[kudu-CR] KUDU-3226 Validate List of Masters for kudu ksck
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/17141 )
Change subject: KUDU-3226 Validate List of Masters for kudu ksck
..
Patch Set 1:
(3 comments)
Thanks for the patch Abhishek!
Looks like you'll also need to update
AdminCliTest.TestAuthzResetCacheIncorrectMasterAddressList in
kudu-admin-test.cc to account for the new error message.
Also, if you scroll down to the bottom of
http://jenkins.kudu.apache.org/job/kudu-gerrit/23429/BUILD_TYPE=LINT/console,
there are a few updates the Kudu linter would like you to make
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc@652
PS1, Line 652: std::
nit: because of the 'using' declarations around L200, this can just be
unordered_set and string below
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc@659
PS1, Line 659: unique_masters.find(master_addresses->at(i)) !=
unique_masters.end()
nit: you can use the ContainsKey() function from gutil/map-util.h
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc@656
PS1, Line 656: for (int i = 0; isize(); i++){
: if ((master_addresses->at(i)).empty())
: continue;
: if (unique_masters.find(master_addresses->at(i)) !=
unique_masters.end()){
: duplicate_masters =
duplicate_masters.append(master_addresses->at(i))+" ";
: }
: else
: unique_masters.insert(master_addresses->at(i));
: }
> It's best to not modify out parameter master_addresses unless we are certai
+1
nit: as well, we generally follow the Google Style Guide, which suggests
consistent usage of curly braces for if/else statements:
https://google.github.io/styleguide/cppguide.html#Conditionals
Same elsewhere.
--
To view, visit http://gerrit.cloudera.org:8080/17141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f
Gerrit-Change-Number: 17141
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 02 Mar 2021 02:15:18 +
Gerrit-HasComments: Yes
[kudu-CR] [java] KUDU-3213: try at different server on TABLET NOT RUNNING
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/17124 ) Change subject: [java] KUDU-3213: try at different server on TABLET_NOT_RUNNING .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17124 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I38ac84a52676ff361fa1ba996665b338d1bbfba1 Gerrit-Change-Number: 17124 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 02 Mar 2021 00:03:12 + Gerrit-HasComments: No
[kudu-CR] KUDU-3226 Validate List of Masters for kudu ksck
Bankim Bhavsar has posted comments on this change. (
http://gerrit.cloudera.org:8080/17141 )
Change subject: KUDU-3226 Validate List of Masters for kudu ksck
..
Patch Set 1:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/17141/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17141/1//COMMIT_MSG@7
PS1, Line 7: KUDU-3226 Validate List of Masters for kudu ksck
To provide context, we also include the component. In this case [tool]
http://gerrit.cloudera.org:8080/#/c/17141/1//COMMIT_MSG@9
PS1, Line 9: This patch checks for duplicate kudu masters reported in kudu
cluster
: ksck/rebalance commands.
So this change is basically validating the input to ksck/rebalance command in
the tool itself before invoking the RPC to the master/tserver, right?
What happens currently without this change?
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/kudu-tool-test.cc@6103
PS1, Line 6103: //Execute a ksck giving the hostname of the local machine twice
: //and it should report the duplicates found.
Nit: space after //
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/kudu-tool-test.cc@6107
PS1, Line 6107: if (!GetHostname(&master_addr).ok())
: master_addr = "";
It'd be better to use ASSERT_OK() and fail the test here in that case. Because
you can't really proceed with "".
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/kudu-tool-test.cc@6110
PS1, Line 6110: Substitute("cluster ksck $0,$0", master_addr)
Nit: For sake of readability it'd be good to put this in a separate line.
Also it'd be good to capture the status explicitly so that we can print the
error in case the assertion is negative.
string cmd = Substitute("cluster ksck $0,$0", master_addr);
Status s = RunActionStderrString(cmd, &out);
ASSERT_TRUE(s.IsRuntimeError()) << s.ToString();
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc@656
PS1, Line 656: for (int i = 0; isize(); i++){
: if ((master_addresses->at(i)).empty())
: continue;
: if (unique_masters.find(master_addresses->at(i)) !=
unique_masters.end()){
: duplicate_masters =
duplicate_masters.append(master_addresses->at(i))+" ";
: }
: else
: unique_masters.insert(master_addresses->at(i));
: }
It's best to not modify out parameter master_addresses unless we are certain
about success and then move the local master addresses to out parameter.
It's better to use the for range loop.
for (const auto& master : master_addresses)
No need to populate the duplicate_masters string unless duplicates are detected
which can be concatenated using strings:Join function.
http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc@667
PS1, Line 667: else
No else clause if there is a return above.
--
To view, visit http://gerrit.cloudera.org:8080/17141
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f
Gerrit-Change-Number: 17141
Gerrit-PatchSet: 1
Gerrit-Owner: Abhishek Chennaka
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 01 Mar 2021 17:46:42 +
Gerrit-HasComments: Yes
[kudu-CR] [client-test] fix TestFailedDnsResolution
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17142 ) Change subject: [client-test] fix TestFailedDnsResolution .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/17142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0493d992c43adb14ef02efae0a15dddc53301d7d Gerrit-Change-Number: 17142 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 01 Mar 2021 17:24:02 + Gerrit-HasComments: No
[kudu-CR] [multi-master-test] fix compilation warning
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17133 ) Change subject: [multi-master-test] fix compilation warning .. [multi-master-test] fix compilation warning Replaced INSTANTIATE_TEST_CASE_P with INSTANTIATE_TEST_SUITE_P in dynamic_multi_master-test.cc to address compilation warning: src/kudu/master/dynamic_multi_master-test.cc:833:1: \ warning: 'InstantiateTestCase_P_IsDeprecated' is deprecated: \ INSTANTIATE_TEST_CASE_P is deprecated, please use \ INSTANTIATE_TEST_SUITE_P [-Wdeprecated-declarations] Change-Id: Ibc2507082e57ada9aea1bdaa7fb0eb11c5e2ced2 Reviewed-on: http://gerrit.cloudera.org:8080/17133 Tested-by: Kudu Jenkins Reviewed-by: Bankim Bhavsar --- M src/kudu/master/dynamic_multi_master-test.cc 1 file changed, 8 insertions(+), 7 deletions(-) Approvals: Kudu Jenkins: Verified Bankim Bhavsar: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/17133 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ibc2507082e57ada9aea1bdaa7fb0eb11c5e2ced2 Gerrit-Change-Number: 17133 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [multi-master-test] fix compilation warning
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/17133 ) Change subject: [multi-master-test] fix compilation warning .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/17133 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibc2507082e57ada9aea1bdaa7fb0eb11c5e2ced2 Gerrit-Change-Number: 17133 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 01 Mar 2021 16:42:56 + Gerrit-HasComments: No
[kudu-CR] [KUDU-3252] a test to reproduce the issue KUDU-3252
Hongjiang Zhang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17143 Change subject: [KUDU-3252] a test to reproduce the issue KUDU-3252 .. [KUDU-3252] a test to reproduce the issue KUDU-3252 This UT sometimes work fine and remove the wal directory when there are 2 FsManager calls CreateInitialFileSystemLayout() I0301 17:41:34.293262 13841 fs_manager.cc:485] Removed file /tmp/kudutest-1000/fs_manager-test.BlockManagerTypes_FsManagerTestBase.TestInitFSInParallel_1.1614591693256906-12747/d1_/instance I0301 17:41:34.293368 13841 fs_manager.cc:485] Removed file /tmp/kudutest-1000/fs_manager-test.BlockManagerTypes_FsManagerTestBase.TestInitFSInParallel_1.1614591693256906-12747/d2_/instance I0301 17:41:34.293431 13841 fs_manager.cc:485] Removed file /tmp/kudutest-1000/fs_manager-test.BlockManagerTypes_FsManagerTestBase.TestInitFSInParallel_1.1614591693256906-12747/d3_/instance I0301 17:41:34.293491 13841 fs_manager.cc:485] Removed file /tmp/kudutest-1000/fs_manager-test.BlockManagerTypes_FsManagerTestBase.TestInitFSInParallel_1.1614591693256906-12747/wal_/instance I0301 17:41:34.293576 13841 fs_manager.cc:494] Removed dir /tmp/kudutest-1000/fs_manager-test.BlockManagerTypes_FsManagerTestBase.TestInitFSInParallel_1.1614591693256906-12747/wal_ Change-Id: I90af630ec0a6917b83bcd16e14c59b5300f770b7 --- M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc 2 files changed, 33 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/17143/1 -- To view, visit http://gerrit.cloudera.org:8080/17143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I90af630ec0a6917b83bcd16e14c59b5300f770b7 Gerrit-Change-Number: 17143 Gerrit-PatchSet: 1 Gerrit-Owner: Hongjiang Zhang
[kudu-CR] [client-test] fix TestFailedDnsResolution
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17142 Change subject: [client-test] fix TestFailedDnsResolution .. [client-test] fix TestFailedDnsResolution I noticed that ClientTest.TestFailedDnsResolution fails unexpectedly with the following error when running on macOS: src/kudu/client/client-test.cc:3205: Failure Value of: s.IsIOError() Actual: false Expected: true unexpected status: OK It turned out that the scenario didn't expect that (a) the results of DNS resolution are cached (b) a tablet server's address can be the same as master's (a) turned true with changelist 48467ccf4, and (b) is true in case of running test mini-cluster with other than UNIQUE_LOOPBACK bind mode: e.g., on macOS it's run in LOOPBACK mode. I updated the scenario to use a non-caching DNS resolver. I also increased the timeout for write operations because the scenario was failing from time to time in case TSAN builds. In addition, since timeouts in GetTableLocations RPC are reported two-fold due to the client's metacache activity, the test was failing rarely due to receiving other non-expected error message. I updated the list of expected error messages to add the missing case. With this patch, the scenario succeeds on macOS and runs more stable for Linux TSAN builds. Change-Id: I0493d992c43adb14ef02efae0a15dddc53301d7d --- M src/kudu/client/client-test.cc 1 file changed, 44 insertions(+), 29 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/17142/1 -- To view, visit http://gerrit.cloudera.org:8080/17142 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I0493d992c43adb14ef02efae0a15dddc53301d7d Gerrit-Change-Number: 17142 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
