[kudu-CR] [rebalancer] location-aware rebalancer (part 1/n)
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/11549 )
Change subject: [rebalancer] location-aware rebalancer (part 1/n)
..
Patch Set 7:
(2 comments)
Two small things then LGTM.
http://gerrit.cloudera.org:8080/#/c/11549/7/src/kudu/tools/placement_policy_util-test.cc
File src/kudu/tools/placement_policy_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/11549/7/src/kudu/tools/placement_policy_util-test.cc@374
PS7, Line 374: // Two tables, each of two RF=3 tablets with replica
distribution violating
: // the placement policy.
This comment doesn't match the test case. The test case has two tables, one of
RF 3 and one of RF 1. Both tablets of the RF=3 tables violate the placement
policy.
http://gerrit.cloudera.org:8080/#/c/11549/7/src/kudu/tools/placement_policy_util-test.cc@472
PS7, Line 472: for (auto idx = 0; idx < configs.size(); ++idx) {
: SCOPED_TRACE(Substitute("test config index: $0", idx));
: const auto& cfg = configs[idx];
:
: ClusterLocalityInfo cli;
: TabletsPlacementInfo tpi;
: ClusterConfigToClusterPlacementInfo(cfg, &cli, &tpi);
:
: vector violations;
: ASSERT_OK(DetectPlacementPolicyViolations(tpi, &violations));
: NO_FATALS(CheckEqual(cfg.reference_violations_info,
violations));
:
: vector moves;
: auto s = FindMovesToReimposePlacementPolicy(tpi, cli,
violations, &moves);
: ASSERT_TRUE(s.IsNotFound()) << s.ToString();
: ASSERT_TRUE(moves.empty());
: }
Couldn't you use RunTest for this?
--
To view, visit http://gerrit.cloudera.org:8080/11549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1cdc3b57a782e6d043b1853600c97fe1f8630347
Gerrit-Change-Number: 11549
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Mon, 29 Oct 2018 17:03:03 +
Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 4/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11745 ) Change subject: [rebalancer] location-aware rebalancer (part 4/n) .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id47183fc853573390b22ec714751adec93e0ea3a Gerrit-Change-Number: 11745 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 17:05:17 + Gerrit-HasComments: No
[kudu-CR] [location awareness] Add location info in ksck report
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11422 ) Change subject: [location_awareness] Add location info in ksck report .. Patch Set 12: (8 comments) http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG@24 PS12, Line 24: Location As for the order of fields, I think keeping the 'Status' before the 'Location' (if counting from the left to right) would make more sense: 1) It's more 'compatible' with the former output (no need to update scripts that are parsing the output, if any exist). 2) The status of a tablet server feels more important field that people would look at usually. 2) Locations most likely to be multi-component hierarchical long strings: it's better to keep that sort of fields in the end of every row. http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG@31 PS12, Line 31: Location | Count nit: maybe, add a title for this table -- 'Tablet Server Location Summary' looks like a good one to me. http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc@1601 PS12, Line 1601: Json nit: why this 'Json' suffix? I see there the plain (non-json) output format is also being verified. http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc@1620 PS12, Line 1620: CheckJsonStringVsKsckResults nit: wrap this into NO_FATALS() ? http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote-test.cc@540 PS12, Line 540: /foo nit: substitute the location from the 'location' variable as well http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote.h File src/kudu/tools/ksck_remote.h: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote.h@95 PS12, Line 95: const nit: what does it buy having const for the HostPort parameter passed by value here? http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_results.cc@365 PS12, Line 365: "Location", "Status" I added a comment on the ordering of these columns: please take a look at the commit message. http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_results.cc@370 PS12, Line 370: location nit: wrap into std::move() -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ideff2dd4975c99a1135002624de2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 12 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 17:14:19 + Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 5/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11746 ) Change subject: [rebalancer] location-aware rebalancer (part 5/n) .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 Gerrit-Change-Number: 11746 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 17:16:57 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 6/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11747 ) Change subject: [rebalancer] location-aware rebalancer (part 6/n) .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11747/1/src/kudu/tools/tool_replica_util.cc File src/kudu/tools/tool_replica_util.cc: http://gerrit.cloudera.org:8080/#/c/11747/1/src/kudu/tools/tool_replica_util.cc@494 PS1, Line 494: s promoted in > I thought making current leader step-down before the non-voter is promoted (thumbsup) -- To view, visit http://gerrit.cloudera.org:8080/11747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I80b560d70c4d7383ee89917a359b4bb2f41bfd31 Gerrit-Change-Number: 11747 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 17:21:55 + Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 7/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11748 ) Change subject: [rebalancer] location-aware rebalancer (part 7/n) .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11748/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11748/2//COMMIT_MSG@12 PS2, Line 12: a Extra "a". -- To view, visit http://gerrit.cloudera.org:8080/11748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ace790aad1c1a4605ef90f6df2104f4a228a5b5 Gerrit-Change-Number: 11748 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 17:34:43 + Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 8/n)
Will Berkeley has posted comments on this change. (
http://gerrit.cloudera.org:8080/11761 )
Change subject: [rebalancer] location-aware rebalancer (part 8/n)
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util-test.cc
File src/kudu/tools/placement_policy_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util-test.cc@633
PS1, Line 633: { "G", { "t0", } }
Extra indentation.
http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util.cc
File src/kudu/tools/placement_policy_util.cc:
http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util.cc@78
PS1, Line 78: edgecases
I'd point out that these are special cases of the general problem of having too
few locations relative to a replication factor and we're calling them out with
a special error message since they are by far the most likely to actually occur.
http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util.cc@81
PS1, Line 81: ==
>=
--
To view, visit http://gerrit.cloudera.org:8080/11761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f8831d254b2ca0d9a12e0ffbc336a59c3c5c8de
Gerrit-Change-Number: 11761
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Mon, 29 Oct 2018 17:40:16 +
Gerrit-HasComments: Yes
[kudu-CR] [master] update placement logic for RF % 2 == 0
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11763 ) Change subject: [master] update placement logic for RF % 2 == 0 .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11763 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4259f805090dd350f31bf3bf2b6477898214ece4 Gerrit-Change-Number: 11763 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 17:48:21 + Gerrit-HasComments: No
[kudu-CR] [examples] Add basic Spark example written in Scala
Will Berkeley has removed a vote on this change. Change subject: [examples] Add basic Spark example written in Scala .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 8 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley
[kudu-CR] [examples] Add basic Spark example written in Scala
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example written in Scala .. Patch Set 8: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 8 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 17:49:12 + Gerrit-HasComments: No
[kudu-CR] KUDU-2584: Prevent flaky off-by-one errors in backup tests
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11815 Change subject: KUDU-2584: Prevent flaky off-by-one errors in backup tests .. KUDU-2584: Prevent flaky off-by-one errors in backup tests This patch adds a one second sleep to the back up and restore tests before a backup is taken. This ensures that we don’t have flakes due to off-by-one errors where all the values are not read. The off-by-one errors are possibly due to very fast test runs where the writes and backups start in under 1ms. They could also be due to some underlying issue that is not well understood. That said in a real world backup scenario, this should have no impact. Further testing/hardening of this job may remove the need for this pause in the future. Change-Id: Ia0f1b4a4138cc8c913543a68fad748927cdc439d --- M java/kudu-backup/src/test/scala/org/apache/kudu/backup/TestKuduBackup.scala 1 file changed, 4 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/11815/1 -- To view, visit http://gerrit.cloudera.org:8080/11815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia0f1b4a4138cc8c913543a68fad748927cdc439d Gerrit-Change-Number: 11815 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] [examples] Add basic Spark example written in Scala
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example written in Scala .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 8 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 18:16:54 + Gerrit-HasComments: No
[kudu-CR] [master] update placement logic for RF % 2 == 0
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11763 ) Change subject: [master] update placement logic for RF % 2 == 0 .. [master] update placement logic for RF % 2 == 0 Updated the logic of the replica placement in master to properly handle even replication factors. Added corresponding unit tests as well. It's possible to configure Kudu to allow creation of tables with an even replication factor. I think it's easier to implement the handling of those cases instead of handling possible inconsistencies in the rebalancer and other tools and adding extra paragraphs into release notes. Change-Id: I4259f805090dd350f31bf3bf2b6477898214ece4 Reviewed-on: http://gerrit.cloudera.org:8080/11763 Tested-by: Kudu Jenkins Reviewed-by: Will Berkeley --- M src/kudu/master/placement_policy-test.cc M src/kudu/master/placement_policy.cc M src/kudu/master/placement_policy.h 3 files changed, 137 insertions(+), 5 deletions(-) Approvals: Kudu Jenkins: Verified Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11763 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I4259f805090dd350f31bf3bf2b6477898214ece4 Gerrit-Change-Number: 11763 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2584: Prevent flaky off-by-one errors in backup tests
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11815 ) Change subject: KUDU-2584: Prevent flaky off-by-one errors in backup tests .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia0f1b4a4138cc8c913543a68fad748927cdc439d Gerrit-Change-Number: 11815 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 18:35:19 + Gerrit-HasComments: No
[kudu-CR] [backup] Port keepAlive implementation to KuduBackupRDD
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11817 Change subject: [backup] Port keepAlive implementation to KuduBackupRDD .. [backup] Port keepAlive implementation to KuduBackupRDD This copies the keepAlive implementation from KuduRDD over to the KuduBackupRDD. Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da --- M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala 2 files changed, 28 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/11817/1 -- To view, visit http://gerrit.cloudera.org:8080/11817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da Gerrit-Change-Number: 11817 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] [examples] Add basic Spark example written in Scala
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example written in Scala .. Patch Set 8: (4 comments) http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@40 PS8, Line 40: - KuduMasters: A String value consisting of a comma-separated list of Kudu Master Host addresses. nit: trailing white space here and below. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@44 PS8, Line 44: If running a spark2-submit job, you will need to set this value to match the '--master' Spark I think this should be spark 2 `spark-submit` job. IIRC spark2-submit is a CDH 5 only command for their compatibility reasons. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@59 PS8, Line 59: To run this against Spark2 On YARN, you can use the spark2-submit command as follows from the Same here. This should just be spark-submit. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@66 PS8, Line 66: $ spark2-submit --class org.apache.kudu.examples.SparkExample --master yarn --deploy-mode --driver-java-options '-DKuduMasters=master.0:7051,master.1:7051,master.2:7051 -DTableName=test_table -DSparkMaster=yarn' target/kudu-spark-example-1.0-SNAPSHOT.jar Same here. This should just be spark-submit. -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 8 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 18:51:43 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2584: Prevent flaky off-by-one errors in backup tests
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11815 ) Change subject: KUDU-2584: Prevent flaky off-by-one errors in backup tests .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11815/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11815/1//COMMIT_MSG@13 PS1, Line 13: : The off-by-one errors are possibly due to very fast : test runs where the writes and backups start : in under 1ms. They could also be due to some : underlying issue that is not well understood. So I thought the underlying issue was due to a different in granularity: Kudu's HT timestamps are specified in micros while the default value for the HT timestamp to backup (System.currentTimeMillis) is specified in millis. The issue being: if the data inserted for the test completes at HT timestamp of "x millis + y micros", the use of currentTimeMillis configures the backup to read at HT timestamp of "x millis", which means the backup misses some of the inserts. After restoring this backup, the number of rows in the table doesn't match the number of rows originally inserted. If this is true, I'd like to see a few changes here: 1) Spell out the issue in detail, both in the commit message and in the corresponding workaround in the test code. 2) Work around the issue in a different way, by calling currentTimeMillis in the Spark job and passing that argument (plus 1 ms) in as the HT timestamp to use for the backup. It's not fundamentally different, but it doesn't slow down the test in the way that Thread.sleep does. -- To view, visit http://gerrit.cloudera.org:8080/11815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia0f1b4a4138cc8c913543a68fad748927cdc439d Gerrit-Change-Number: 11815 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 18:53:55 + Gerrit-HasComments: Yes
[kudu-CR] [backup] Port keepAlive implementation to KuduBackupRDD
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11817 ) Change subject: [backup] Port keepAlive implementation to KuduBackupRDD .. Patch Set 1: (1 comment) I'd ask you to port the test too, but that's even more code duplication. How difficult would it be to combine the RDD implementations at this point? http://gerrit.cloudera.org:8080/#/c/11817/1/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala: http://gerrit.cloudera.org:8080/#/c/11817/1/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@90 PS1, Line 90: he the -- To view, visit http://gerrit.cloudera.org:8080/11817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da Gerrit-Change-Number: 11817 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 29 Oct 2018 18:55:51 + Gerrit-HasComments: Yes
[kudu-CR] [examples] Add basic Spark example written in Scala
Mitch Barnett has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example written in Scala .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@40 PS8, Line 40: - KuduMasters: A String value consisting of a comma-separated list of Kudu Master Host addresses. > nit: trailing white space here and below. Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@44 PS8, Line 44: If running a spark2-submit job, you will need to set this value to match the '--master' Spark > I think this should be spark 2 `spark-submit` job. IIRC spark2-submit is a Yeah, the upstream is just 'spark-submit' but I thought we wanted this targeted for CDH - per Greg's update. Should I leave it as is to be CDH-specific, or change it to match upstream spark conventions? -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 8 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 18:57:06 + Gerrit-HasComments: Yes
[kudu-CR] [backup] Port keepAlive implementation to KuduBackupRDD
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11817 ) Change subject: [backup] Port keepAlive implementation to KuduBackupRDD .. Patch Set 1: I would really like to combine the implementations. The one thing holding us back is the open question about incremental backups impact. It would not be hard to merge. -- To view, visit http://gerrit.cloudera.org:8080/11817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da Gerrit-Change-Number: 11817 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 29 Oct 2018 18:58:43 + Gerrit-HasComments: No
[kudu-CR] [backup] Port keepAlive implementation to KuduBackupRDD
Hello Mike Percy, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11817 to look at the new patch set (#2). Change subject: [backup] Port keepAlive implementation to KuduBackupRDD .. [backup] Port keepAlive implementation to KuduBackupRDD This copies the keepAlive implementation from KuduRDD over to the KuduBackupRDD. Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da --- M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala 2 files changed, 28 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/11817/2 -- To view, visit http://gerrit.cloudera.org:8080/11817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da Gerrit-Change-Number: 11817 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] [examples] Add basic Spark example written in Scala
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example written in Scala .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@44 PS8, Line 44: If running a spark2-submit job, you will need to set this value to match the '--master' Spark > Yeah, the upstream is just 'spark-submit' but I thought we wanted this targ This is an Apache project and an Apache repository. Nothing about this change should be related to or specifically for CDH. All changes should target the Apache integrations. -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 8 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 19:02:46 + Gerrit-HasComments: Yes
[kudu-CR] [backup] Port keepAlive implementation to KuduBackupRDD
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11817 ) Change subject: [backup] Port keepAlive implementation to KuduBackupRDD .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11817/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala: http://gerrit.cloudera.org:8080/#/c/11817/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@90 PS2, Line 90: The Should be lower-case. -- To view, visit http://gerrit.cloudera.org:8080/11817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da Gerrit-Change-Number: 11817 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 29 Oct 2018 19:03:20 + Gerrit-HasComments: Yes
[kudu-CR] deltas: fuzz tests for delta file and dms
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11140 ) Change subject: deltas: fuzz tests for delta file and dms .. Patch Set 15: (8 comments) looks good overall http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h File src/kudu/tablet/tablet-test-util.h: http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@354 PS15, Line 354:T::kTag == REDO, :std::less, :std::greater>::type; nit: unusual indentation http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@454 PS15, Line 454: // Transforms and writes deltas into 'deltas', a vector of "delta lists", each please add a description of the semantics of this method (what it's doing / why it should be called); only technical details have been provided so far in this comment maybe just: Collect all mutations relevant to a scan at timestamp ts. http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@467 PS15, Line 467: continue break? http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@750 PS15, Line 750: // Runs a fuzz test by generating a fairly random DeltaIterator, using it to Clarify what component is being tested here http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@772 PS15, Line 772: inclusive snapshot what does this mean? the prng->Uniform() logic below treats .second as exclusive http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@810 PS15, Line 810: - 1 nit: no need for this -1 because Random::Uniform() returns values from [0 .. n-1] http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.cc File src/kudu/tablet/tablet-test-util.cc: http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.cc@26 PS15, Line 26: to_include > ts; nit: maybe it's just me, but I usually think of this as ts < to_include, because to_include is sort of fixed, but obviously it's the same thing so feel free to ignore this http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.cc@32 PS15, Line 32: to_include <= ts same here, I usually think of this as ts >= to_include -- To view, visit http://gerrit.cloudera.org:8080/11140 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I539ff0f2055cf398a42efb824238188230e25516 Gerrit-Change-Number: 11140 Gerrit-PatchSet: 15 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 29 Oct 2018 19:06:12 + Gerrit-HasComments: Yes
[kudu-CR] [examples] Add basic Spark example (scala)
Mitch Barnett has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example (scala) .. Patch Set 9: (3 comments) http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@44 PS8, Line 44: If running a spark2 'spark-submit' job, you will need to set this value to match the '--master' Spark > This is an Apache project and an Apache repository. Nothing about this chan Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@59 PS8, Line 59: To run this as a spark2 'spark-submit' job, you can use the spark-submit command as follows from the > Same here. This should just be spark-submit. Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@66 PS8, Line 66: $ spark-submit --class org.apache.kudu.examples.SparkExample --master yarn --deploy-mode --driver-java-options '-DKuduMasters=master.0:7051,master.1:7051,master.2:7051 -DTableName=test_table -DSparkMaster=yarn' target/kudu-spark-example-1.0-SNAPSHOT.jar > Same here. This should just be spark-submit. Done -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 9 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 19:23:44 + Gerrit-HasComments: Yes
[kudu-CR] [examples] Add basic Spark example (scala)
Hello Will Berkeley, Attila Bukor, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11788 to look at the new patch set (#9). Change subject: [examples] Add basic Spark example (scala) .. [examples] Add basic Spark example (scala) This patch adds a basic Kudu client that utilizes both Kudu Java APIs, as well as Spark SQL APIs. It will allow customers to pull down the pom.xml and scala source, then build and execute from their local machine. Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f --- A examples/scala/spark-example/README.adoc A examples/scala/spark-example/pom.xml A examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala 3 files changed, 292 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/11788/9 -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 9 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley
[kudu-CR] [examples] Add basic Spark example (scala)
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example (scala) .. Patch Set 8: (1 comment) http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@44 PS8, Line 44: If running a spark2-submit job, you will need to set this value to match the '--master' Spark > Yeah, the upstream is just 'spark-submit' but I thought we wanted this targ To clarify, my point was not to make this example CDH-specific, but to make it easy to run on CDH or HDP, or really any other distro that makes it easy way to install and configure Spark+Yarn+Hive+Kudu for a noob, especially for someone with little previous Kudu, Spark, Hive and Yarn experience. LGTM with "spark-submit". -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 8 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 19:39:00 + Gerrit-HasComments: Yes
[kudu-CR] [examples] Add basic Spark example (scala)
Mitch Barnett has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example (scala) .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@44 PS8, Line 44: If running a spark2 'spark-submit' job, you will need to set this value to match the '--master' Spark > To clarify, my point was not to make this example CDH-specific, but to make Gotcha - misunderstanding on my part! -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 9 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 19:47:04 + Gerrit-HasComments: Yes
[kudu-CR] [examples] Add basic Spark example (scala)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example (scala) .. Patch Set 9: (44 comments) Still need to try running it. Will respond again after I have. http://gerrit.cloudera.org:8080/#/c/11788/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11788/8//COMMIT_MSG@9 PS8, Line 9: Kudu client You mean it adds a basic kudu-spark example? http://gerrit.cloudera.org:8080/#/c/11788/8//COMMIT_MSG@11 PS8, Line 11: It will allow customers to pull down the pom.xml and scala source, nit: No paragraph break. http://gerrit.cloudera.org:8080/#/c/11788/8//COMMIT_MSG@11 PS8, Line 11: customers Say "users" instead of "customers". Nobody pays Apache to use Apache Kudu. Or, if they do, I'm not getting my take and I'm pretty mad -.- http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@22 PS8, Line 22: Kudu client APIs This is misleading. It doesn't use the KuduClient directly. Instead it uses the methods on KuduContext, which is the recommended way to do things. I would say instead that the program "uses the Kudu-Spark integration to:" http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@27 PS8, Line 27: Scan some rows and then split this list item into two: - Scan rows using RDD / DataFrame methods - Scan rows using SparkSQL http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@37 PS8, Line 37: The above runs against a local cluster? What does it do? http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@38 PS8, Line 38: configurable parameters More precisely, these are Java system properties. Say that, since many users will understand what this means. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@40 PS8, Line 40: Host Remove http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@40 PS8, Line 40: Master nit: master http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@40 PS8, Line 40: A String value consisting of Remove this first part of the sentence. Parameters from the command line always start as strings. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@41 PS8, Line 41: This Remove, so it's just "Defaults to..." http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@41 PS8, Line 41: This will need to be pointed to the Kudu cluster you wish to target. I think this is obvious and should be omitted. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@43 PS8, Line 43: location You mean the address, right? http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@43 PS8, Line 43: A String value conatining Remove. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@46 PS8, Line 46: A String value containing Remove. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@47 PS8, Line 47: The default table name is Replace with "Defaults to". http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@49 PS8, Line 49: To specify a value at execution time, you'll specify the parameter name in the '-D' format. Omit this sentence since it's a generality about Java system properties, and the example demonstrates the syntax anyway. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@51 PS8, Line 51: set the property `KuduMasters` to a CSV of the master addresses in the form : `host:port` and add a table name value, as shown: Replace with just "use a command like the following:" http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@56 PS8, Line 56: KuduMasters I don't like PascalCase for these parameters. We have the same kuduMaster parameter for the java example except it's named camelCase. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@56 PS8, Line 56: TableName tableName http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@56 PS8, Line 56: target/kudu-spark-example-1.0-SNAPSHOT.jar Split the command so it's 80 or less characters per line. Remember to use \ to break bash commands into multiple lines properly. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@59 PS8, Line 59: rk2 'spark-sub Do we have to care whether Spark is running on YARN vs anything else? Seems like the answer should be no. What do you do if you are running on Mesos? Is that still a thing for
[kudu-CR] KUDU-2584: Prevent flaky off-by-one errors in backup tests
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11815 ) Change subject: KUDU-2584: Prevent flaky off-by-one errors in backup tests .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/11815/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11815/1//COMMIT_MSG@13 PS1, Line 13: : The off-by-one errors are possibly due to very fast : test runs where the writes and backups start : in under 1ms. They could also be due to some : underlying issue that is not well understood. > So I thought the underlying issue was due to a different in granularity: Ku +1 I am suspicious of fixing a test failure like this with a sleep, and particularly so in this case because the commit message doesn't make it clear to me why the sleep is truly fixing the issue and not just covering it up. If Adar's analysis is right then I think his proposed fix makes sense. http://gerrit.cloudera.org:8080/#/c/11815/1//COMMIT_MSG@20 PS1, Line 20: dist-test works with Java now right? Can you run some experiments to demonstrate this patch fixes the problem and that it reduces the flakiness of the test in general? -- To view, visit http://gerrit.cloudera.org:8080/11815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia0f1b4a4138cc8c913543a68fad748927cdc439d Gerrit-Change-Number: 11815 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 20:09:37 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add location info in ksck report
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11422 ) Change subject: [location_awareness] Add location info in ksck report .. Patch Set 12: (3 comments) The location isn't getting piped through to the JSON output. You'll also need to . Add the location to the KsckServerHealthSummaryPB in tool.proto. . Set the location field in the proto in KsckServerHealthSummaryToPb in ksck_results.cc . Enhance CheckJsonVsServerHealthSummaries in ksck-test.cc so it checks the locations set in the json vs the locations set in the summaries. http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG@24 PS12, Line 24: Location > As for the order of fields, I think keeping the 'Status' before the 'Locati +1 http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG@31 PS12, Line 31: Location | Count > nit: maybe, add a title for this table -- 'Tablet Server Location Summary' +1 http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote.h File src/kudu/tools/ksck_remote.h: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote.h@95 PS12, Line 95: const > nit: what does it buy having const for the HostPort parameter passed by val It should either by a const reference or non-const and passed-by-value with std::move if HostPort has a move constructor. -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ideff2dd4975c99a1135002624de2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 12 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 20:18:15 + Gerrit-HasComments: Yes
[kudu-CR] [backup] Port keepAlive implementation to KuduBackupRDD
Hello Mike Percy, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11817 to look at the new patch set (#3). Change subject: [backup] Port keepAlive implementation to KuduBackupRDD .. [backup] Port keepAlive implementation to KuduBackupRDD This copies the keepAlive implementation from KuduRDD over to the KuduBackupRDD. Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da --- M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala 2 files changed, 28 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/17/11817/3 -- To view, visit http://gerrit.cloudera.org:8080/11817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da Gerrit-Change-Number: 11817 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] [backup] Port keepAlive implementation to KuduBackupRDD
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11817 ) Change subject: [backup] Port keepAlive implementation to KuduBackupRDD .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da Gerrit-Change-Number: 11817 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 29 Oct 2018 20:27:24 + Gerrit-HasComments: No
[kudu-CR] [backup] Port keepAlive implementation to KuduBackupRDD
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11817 ) Change subject: [backup] Port keepAlive implementation to KuduBackupRDD .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11817/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala: http://gerrit.cloudera.org:8080/#/c/11817/2/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupOptions.scala@90 PS2, Line 90: The > Should be lower-case. Done -- To view, visit http://gerrit.cloudera.org:8080/11817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da Gerrit-Change-Number: 11817 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 29 Oct 2018 20:31:28 + Gerrit-HasComments: Yes
[kudu-CR] [backup] Port keepAlive implementation to KuduBackupRDD
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/11817 ) Change subject: [backup] Port keepAlive implementation to KuduBackupRDD .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/11817/3/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala: http://gerrit.cloudera.org:8080/#/c/11817/3/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@113 PS3, Line 113: private var lastKeepAliveTimeMs = System.currentTimeMillis() I guess this applies to the other patch as well; shouldn't we use nanotime? Here and below. To avoid being affected by clock changes. -- To view, visit http://gerrit.cloudera.org:8080/11817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I10d7645781d2b18eb89034e1b4f55045b01ca0da Gerrit-Change-Number: 11817 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Comment-Date: Mon, 29 Oct 2018 20:43:16 + Gerrit-HasComments: Yes
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Hello Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11819 to review the following change. Change subject: rpc-test: fix TestClientConnectionMetrics .. rpc-test: fix TestClientConnectionMetrics Every now and then, this test would fail with: rpc-test.cc:542: Failure Expected: (dump_resp.outbound_connections(0).outbound_queue_size()) > (0), actual: 0 vs 0 Unfortunately, the test would go one to crash (and trigger a TSAN warning) due to the lack of proper cleanup in the event of an ASSERT failure. I've fixed that in this patch. I also tried to address the root of the test flakiness (that the outbound transfer queue contains at least one element), but I couldn't find a good way to do it. Blocking the server reactor thread has no effect on client-side queuing. And we can't block the client reactor thread outright because DumpRunningRpcs runs on it. Some of this is touched on in the original code review[1] that committed the test. Having given up, I wrapped the whole thing in an ASSERT_EVENTUALLY. It's ham-fisted for sure, but it seems to work: without it, the test fails every 100-200 runs on my laptop, and with it I can't get it to fail at all. I also looped it 1000 times in TSAN mode with 8 stress threads and didn't see any failures. I don't understand the krpc subsystem very well, so if there's a better way, I'm all ears. 1. https://gerrit.cloudera.org/c/9343/ Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 --- M src/kudu/rpc/rpc-test.cc 1 file changed, 45 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/11819/1 -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Todd Lipcon
[kudu-CR] catalog manager: fix a TSAN race
Hello Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/11818 to review the following change. Change subject: catalog_manager: fix a TSAN race .. catalog_manager: fix a TSAN race Nnoticed this on the flaky test dashboard for alter_table-randomized-test; see the end of the commit message for the complete output. I also removed an unrelated and unnecessary lock acquisition. To test, I looped alter_table_randomized-test in slow mode with TSAN and the two failures I saw did not report any data races. WARNING: ThreadSanitizer: data race (pid=17016) Read of size 8 at 0x7b4c10d0 by thread T68 (mutexes: write M1500): #0 std::__1::unique_ptr >::operator bool() const /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/c++/v1/memory:2583:19 (libmaster.so+0xb99b1) #1 kudu::master::CatalogManager::PrepareForLeadershipTask() /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/master/catalog_manager.cc:1055 (libmaster.so+0xb99b1) #2 kudu::internal::RunnableAdapter::Run(kudu::master::CatalogManager*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/bind_internal.h:136:12 (libmaster.so+0x102fa9) #3 kudu::internal::InvokeHelper, void ()(kudu::master::CatalogManager*)>::MakeItSo(kudu::internal::RunnableAdapter, kudu::master::CatalogManager*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/bind_internal.h:873:14 (libmaster.so+0x102ec5) #4 kudu::internal::Invoker<1, kudu::internal::BindState, void ()(kudu::master::CatalogManager*), void ()(kudu::internal::UnretainedWrapper)>, void ()(kudu::master::CatalogManager*)>::Run(kudu::internal::BindStateBase*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/bind_internal.h:1065:12 (libmaster.so+0x102e0a) #5 kudu::Callback::Run() const /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/callback.h:396:12 (libconsensus.so+0xa6dfd) #6 kudu::ClosureRunnable::Run() /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/util/threadpool.cc:76:9 (libkudu_util.so+0x1cc9ad) #7 kudu::ThreadPool::DispatchThread() /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/util/threadpool.cc:686:22 (libkudu_util.so+0x1c86d8) #8 boost::_mfi::mf0::operator()(kudu::ThreadPool*) const /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/boost/bind/mem_fn_template.hpp:49:29 (libkudu_util.so+0x1d3649) #9 void boost::_bi::list1 >::operator(), boost::_bi::list0>(boost::_bi::type, boost::_mfi::mf0&, boost::_bi::list0&, int) /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/boost/bind/bind.hpp:259:9 (libkudu_util.so+0x1d359a) #10 boost::_bi::bind_t, boost::_bi::list1 > >::operator()() /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/boost/bind/bind.hpp:1222:16 (libkudu_util.so+0x1d3523) #11 boost::detail::function::void_function_obj_invoker0, boost::_bi::list1 > >, void>::invoke(boost::detail::function::function_buffer&) /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/boost/function/function_template.hpp:159:11 (libkudu_util.so+0x1d3319) #12 boost::function0::operator()() const /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/boost/function/function_template.hpp:770:14 (libkrpc.so+0xb6651) #13 kudu::Thread::SuperviseThread(void*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/util/thread.cc:615:3 (libkudu_util.so+0x1bfe34) Previous write of size 8 at 0x7b4c10d0 by thread T59: #0 std::__1::unique_ptr >::reset(kudu::hms::HmsCatalog*) /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/c++/v1/memory:2596:20 (libmaster.so+0xb8b6f) #1 kudu::master::CatalogManager::Init(bool) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/master/catalog_manager.cc:730 (libmaster.so+0xb8b6f) #2 kudu::master::Master::InitCatalogManager() /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/master/master.cc:216:3 (libmaster.so+0x11fa5f) #3 kudu::master::Master::InitCatalogManagerTask() /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/master/master.cc:205:14 (libmaster.so+0x11f8b2) #4 kudu::internal::RunnableAdapter::Run(kudu::master::Master*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/bind_internal.h:136:12 (libmaster.so+0x124449) #5 kudu::internal::InvokeHelper, void ()(kudu::master::Master*)>::MakeItSo(kudu::internal::RunnableAdapter, kudu::master::Master*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/bind_internal.h:873:14 (libmaster.so+0x124365) #6 kudu::internal::Invoker<1, kudu::internal::BindState, void ()(kudu::master::Master*), void ()(kudu::internal:
[kudu-CR] [rebalancer] location-aware rebalancer (part 7/n)
Hello Will Berkeley, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11748 to look at the new patch set (#3). Change subject: [rebalancer] location-aware rebalancer (part 7/n) .. [rebalancer] location-aware rebalancer (part 7/n) Added PolicyFixer and integrated the cross-location balancing algorithm. Added one integration test as well, but it's disabled for now since it requires a tablet server's location to be reported in KsckServerHealthSummary. If that piece is in place, the test passes. More integration tests will be added in a follow-up commit. Change-Id: I9ace790aad1c1a4605ef90f6df2104f4a228a5b5 --- M src/kudu/tools/rebalancer.cc M src/kudu/tools/rebalancer.h M src/kudu/tools/rebalancer_tool-test.cc M src/kudu/tools/tool_action_cluster.cc 4 files changed, 709 insertions(+), 49 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/48/11748/3 -- To view, visit http://gerrit.cloudera.org:8080/11748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9ace790aad1c1a4605ef90f6df2104f4a228a5b5 Gerrit-Change-Number: 11748 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 8/n)
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11761 )
Change subject: [rebalancer] location-aware rebalancer (part 8/n)
..
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util-test.cc
File src/kudu/tools/placement_policy_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util-test.cc@633
PS1, Line 633: { "G", { "t0", } }
> Extra indentation.
Done
http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util.cc
File src/kudu/tools/placement_policy_util.cc:
http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util.cc@78
PS1, Line 78: edgecases
> I'd point out that these are special cases of the general problem of having
Done
http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util.cc@81
PS1, Line 81: ==
> >=
Well, I think this case is a separate one.
It seems with 3 locations and RF=5 there is a good way to place replicas so the
majority would be on-line if any tablet servers goes down:
2+2+1
In case of 3 locations and RF=6 that's
2+2+2
And so on with higher replication factors.
--
To view, visit http://gerrit.cloudera.org:8080/11761
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f8831d254b2ca0d9a12e0ffbc336a59c3c5c8de
Gerrit-Change-Number: 11761
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Mon, 29 Oct 2018 20:54:57 +
Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 8/n)
Hello Will Berkeley, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11761 to look at the new patch set (#2). Change subject: [rebalancer] location-aware rebalancer (part 8/n) .. [rebalancer] location-aware rebalancer (part 8/n) Updated FindBestReplicaToReplace() to handle common and edge cases of even replication factors. Added corresponding unit tests as well. Change-Id: I8f8831d254b2ca0d9a12e0ffbc336a59c3c5c8de --- M src/kudu/tools/placement_policy_util-test.cc M src/kudu/tools/placement_policy_util.cc 2 files changed, 205 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/11761/2 -- To view, visit http://gerrit.cloudera.org:8080/11761 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8f8831d254b2ca0d9a12e0ffbc336a59c3c5c8de Gerrit-Change-Number: 11761 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 5/n)
Hello Will Berkeley, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11746 to look at the new patch set (#3). Change subject: [rebalancer] location-aware rebalancer (part 5/n) .. [rebalancer] location-aware rebalancer (part 5/n) Added LocationBalancingAlgo and corresponding units tests. Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 --- M src/kudu/tools/rebalance_algo-test.cc M src/kudu/tools/rebalance_algo.cc M src/kudu/tools/rebalance_algo.h 3 files changed, 623 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/11746/3 -- To view, visit http://gerrit.cloudera.org:8080/11746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 Gerrit-Change-Number: 11746 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 1/n)
Hello Will Berkeley, Tidy Bot, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11549 to look at the new patch set (#8). Change subject: [rebalancer] location-aware rebalancer (part 1/n) .. [rebalancer] location-aware rebalancer (part 1/n) Added logic to identify and fix placement policy violations. Also, added units tests to cover the new functionality. Change-Id: I1cdc3b57a782e6d043b1853600c97fe1f8630347 --- M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_results.h A src/kudu/tools/placement_policy_util-test.cc A src/kudu/tools/placement_policy_util.cc A src/kudu/tools/placement_policy_util.h M src/kudu/tools/rebalance_algo.h M src/kudu/tools/rebalancer.h 7 files changed, 1,001 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/11549/8 -- To view, visit http://gerrit.cloudera.org:8080/11549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1cdc3b57a782e6d043b1853600c97fe1f8630347 Gerrit-Change-Number: 11549 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 1/n)
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11549 )
Change subject: [rebalancer] location-aware rebalancer (part 1/n)
..
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11549/7/src/kudu/tools/placement_policy_util-test.cc
File src/kudu/tools/placement_policy_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/11549/7/src/kudu/tools/placement_policy_util-test.cc@374
PS7, Line 374: // Two tables, each of two RF=3 tablets with replica
distribution violating
: // the placement policy.
> This comment doesn't match the test case. The test case has two tables, one
That's a good catch!
http://gerrit.cloudera.org:8080/#/c/11549/7/src/kudu/tools/placement_policy_util-test.cc@472
PS7, Line 472: for (auto idx = 0; idx < configs.size(); ++idx) {
: SCOPED_TRACE(Substitute("test config index: $0", idx));
: const auto& cfg = configs[idx];
:
: ClusterLocalityInfo cli;
: TabletsPlacementInfo tpi;
: ClusterConfigToClusterPlacementInfo(cfg, &cli, &tpi);
:
: vector violations;
: ASSERT_OK(DetectPlacementPolicyViolations(tpi, &violations));
: NO_FATALS(CheckEqual(cfg.reference_violations_info,
violations));
:
: vector moves;
: auto s = FindMovesToReimposePlacementPolicy(tpi, cli,
violations, &moves);
: ASSERT_TRUE(s.IsNotFound()) << s.ToString();
: ASSERT_TRUE(moves.empty());
: }
> Couldn't you use RunTest for this?
This piece a bit special: it checks for Status::NotFound() from
FindMovesToReimposePlacementPolicy() (not from anything else), while RunTest()
is using compound-like approach. I would prefer to keep that separate.
--
To view, visit http://gerrit.cloudera.org:8080/11549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1cdc3b57a782e6d043b1853600c97fe1f8630347
Gerrit-Change-Number: 11549
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Will Berkeley
Gerrit-Comment-Date: Mon, 29 Oct 2018 20:55:39 +
Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 7/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11748 ) Change subject: [rebalancer] location-aware rebalancer (part 7/n) .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11748/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11748/2//COMMIT_MSG@12 PS2, Line 12: a > Extra "a". Done -- To view, visit http://gerrit.cloudera.org:8080/11748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ace790aad1c1a4605ef90f6df2104f4a228a5b5 Gerrit-Change-Number: 11748 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 20:55:02 + Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 5/n)
Hello Will Berkeley, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11746 to look at the new patch set (#4). Change subject: [rebalancer] location-aware rebalancer (part 5/n) .. [rebalancer] location-aware rebalancer (part 5/n) Added LocationBalancingAlgo and corresponding units tests. Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 --- M src/kudu/tools/rebalance_algo-test.cc M src/kudu/tools/rebalance_algo.cc M src/kudu/tools/rebalance_algo.h 3 files changed, 623 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/11746/4 -- To view, visit http://gerrit.cloudera.org:8080/11746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 Gerrit-Change-Number: 11746 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/11819 )
Change subject: rpc-test: fix TestClientConnectionMetrics
..
Patch Set 1:
(2 comments)
Looks good, just a couple of nits.
http://gerrit.cloudera.org:8080/#/c/11819/1/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/11819/1/src/kudu/rpc/rpc-test.cc@521
PS1, Line 521: int n_calls = 1000
nit: add constexpr ?
http://gerrit.cloudera.org:8080/#/c/11819/1/src/kudu/rpc/rpc-test.cc@552
PS1, Line 552: // Verify that all the RPCs have finished.
: for (const auto& controller : controllers) {
: ASSERT_TRUE(controller->finished());
: }
nit: does it make sense to move this piece out of the ASSERT_EVENTUALLY() after
wait at latch is done? This should not fail if the condition of the
ASSERT_GT() at L546 is true, right?
--
To view, visit http://gerrit.cloudera.org:8080/11819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980
Gerrit-Change-Number: 11819
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Mon, 29 Oct 2018 21:37:28 +
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: fix a TSAN race
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11818 ) Change subject: catalog_manager: fix a TSAN race .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/11818/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11818/1//COMMIT_MSG@9 PS1, Line 9: n nit: an extra 'n' http://gerrit.cloudera.org:8080/#/c/11818/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11818/1/src/kudu/master/catalog_manager.cc@735 PS1, Line 735:std::lock_guard leader_lock_guard(leader_lock_); It seems the lock will be released once the outer scope ends at line 744 of the new revision, so background_tasks_ would not be protected anymore by this lock. Is this intentional? -- To view, visit http://gerrit.cloudera.org:8080/11818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I090a832b7fb25d8cb1e770c025048f73ac997eac Gerrit-Change-Number: 11818 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 29 Oct 2018 21:45:02 + Gerrit-HasComments: Yes
[kudu-CR] [examples] Add basic Spark example (scala)
Hello Will Berkeley, Attila Bukor, Kudu Jenkins, Adar Dembo, Grant Henke, Greg Solovyev, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11788 to look at the new patch set (#10). Change subject: [examples] Add basic Spark example (scala) .. [examples] Add basic Spark example (scala) This patch adds a basic Kudu-Spark example that utilizes the Kudu-Spark integration. It will allow users to pull down the pom.xml and scala source, then build and execute from their local machine. Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f --- A examples/scala/spark-example/README.adoc A examples/scala/spark-example/pom.xml A examples/scala/spark-example/src/main/scala/org/apache/kudu/examples/SparkExample.scala 3 files changed, 298 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/11788/10 -- To view, visit http://gerrit.cloudera.org:8080/11788 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9ba09f0118c054a07b951e241c31d66245c57d3f Gerrit-Change-Number: 11788 Gerrit-PatchSet: 10 Gerrit-Owner: Mitch Barnett Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mitch Barnett Gerrit-Reviewer: Will Berkeley
[kudu-CR] [examples] Add basic Spark example (scala)
Mitch Barnett has posted comments on this change. ( http://gerrit.cloudera.org:8080/11788 ) Change subject: [examples] Add basic Spark example (scala) .. Patch Set 10: (41 comments) http://gerrit.cloudera.org:8080/#/c/11788/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11788/8//COMMIT_MSG@9 PS8, Line 9: Kudu-Spark > You mean it adds a basic kudu-spark example? I think I copied this from the Kudu-Java example patch notes Fixed http://gerrit.cloudera.org:8080/#/c/11788/8//COMMIT_MSG@11 PS8, Line 11: It will allow users to pull down the pom.xml and scala source, > nit: No paragraph break. Pushing to gerrit throws a warn message about line breaks, which is why I added this in. Safe to ignore and leave it as a single line without width considerations, or is there a better way to meet the width limitation? http://gerrit.cloudera.org:8080/#/c/11788/8//COMMIT_MSG@11 PS8, Line 11: users to > Say "users" instead of "customers". Nobody pays Apache to use Apache Kudu. Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc File examples/scala/spark-example/README.adoc: http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@22 PS8, Line 22: the Kudu-Spark i > This is misleading. It doesn't use the KuduClient directly. Instead it uses Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@27 PS8, Line 27: Scan some rows > and then split this list item into two: Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@37 PS8, Line 37: $ mvn package > The above runs against a local cluster? What does it do? Not quite - it will run as a local execution, and build the 'spark' architecture locally when executed. There's no cluster to run against here, it's effectively a standalone application running in it's own JVM. http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@38 PS8, Line 38: et/kudu-spark-example-1 > More precisely, these are Java system properties. Say that, since many user Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@40 PS8, Line 40: > Remove Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@40 PS8, Line 40: > Remove this first part of the sentence. Parameters from the command line al Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@40 PS8, Line 40: > nit: master Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@41 PS8, Line 41: ere are a few Java system properties defined in SparkExample.scala: > I think this is obvious and should be omitted. Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@41 PS8, Line 41: > Remove, so it's just "Defaults to..." Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@43 PS8, Line 43: A comma-separated list of > Remove. Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@43 PS8, Line 43: master > You mean the address, right? Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@46 PS8, Line 46: park2 'spark-submit' job, > Remove. Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@47 PS8, Line 47: the '--master' Spark para > Replace with "Defaults to". Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@49 PS8, Line 49: Defaults to 'spark_test'. > Omit this sentence since it's a generality about Java system properties, an Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@51 PS8, Line 51: different set of masters for the Kudu cluster from the : command line and use a custom table name, use a c > Replace with just "use a command like the following:" Done http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@59 PS8, Line 59: > Do we have to care whether Spark is running on YARN vs anything else? Seems Yep, this was corrected in a later revision. I misunderstood some of the previous comments and made this a bit too CDH-centric. This should now just read: "To run this as a spark2 'spark-submit' job" http://gerrit.cloudera.org:8080/#/c/11788/8/examples/scala/spark-example/README.adoc@60 PS8, Line 60: ubmit command > What does this mean? In the spark-submit, you supply the spark-master address as part of the command-line options. In order to make this work between both spark-submit and also a standalone java execution, I need to have the '--master' value from the spark options also be supplied to the Kudu-Spark example via '-DSparkMaster='. Can explain in more detail in chat/call if needed. http://gerrit.c
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11819 )
Change subject: rpc-test: fix TestClientConnectionMetrics
..
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11819/1/src/kudu/rpc/rpc-test.cc
File src/kudu/rpc/rpc-test.cc:
http://gerrit.cloudera.org:8080/#/c/11819/1/src/kudu/rpc/rpc-test.cc@521
PS1, Line 521: int n_calls = 1000
> nit: add constexpr ?
Done
http://gerrit.cloudera.org:8080/#/c/11819/1/src/kudu/rpc/rpc-test.cc@552
PS1, Line 552: // Verify that all the RPCs have finished.
: for (const auto& controller : controllers) {
: ASSERT_TRUE(controller->finished());
: }
> nit: does it make sense to move this piece out of the ASSERT_EVENTUALLY() a
Can't; the controllers are defined within the ASSERT_EVENTUALLY, and since they
need to be replaced with each call, it'd be messy to decouple them from the
calls themselves.
--
To view, visit http://gerrit.cloudera.org:8080/11819
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980
Gerrit-Change-Number: 11819
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Mon, 29 Oct 2018 22:38:43 +
Gerrit-HasComments: Yes
[kudu-CR] catalog manager: fix a TSAN race
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11818 ) Change subject: catalog_manager: fix a TSAN race .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/11818/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11818/1//COMMIT_MSG@9 PS1, Line 9: n > nit: an extra 'n' Oops, fixed. http://gerrit.cloudera.org:8080/#/c/11818/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11818/1/src/kudu/master/catalog_manager.cc@735 PS1, Line 735:std::lock_guard leader_lock_guard(leader_lock_); > It seems the lock will be released once the outer scope ends at line 744 of Yes, this was intentional. I don't believe background_tasks.reset() needs any additional synchronization; only hms_catalog_ needs to be synchronized due to its read within PrepareForLeadershipTask. -- To view, visit http://gerrit.cloudera.org:8080/11818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I090a832b7fb25d8cb1e770c025048f73ac997eac Gerrit-Change-Number: 11818 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 29 Oct 2018 22:39:55 + Gerrit-HasComments: Yes
[kudu-CR] catalog manager: fix a TSAN race
Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11818 to look at the new patch set (#2). Change subject: catalog_manager: fix a TSAN race .. catalog_manager: fix a TSAN race Noticed this on the flaky test dashboard for alter_table-randomized-test; see the end of the commit message for the complete output. I also removed an unrelated and unnecessary lock acquisition. To test, I looped alter_table_randomized-test in slow mode with TSAN and the two failures I saw did not report any data races. WARNING: ThreadSanitizer: data race (pid=17016) Read of size 8 at 0x7b4c10d0 by thread T68 (mutexes: write M1500): #0 std::__1::unique_ptr >::operator bool() const /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/c++/v1/memory:2583:19 (libmaster.so+0xb99b1) #1 kudu::master::CatalogManager::PrepareForLeadershipTask() /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/master/catalog_manager.cc:1055 (libmaster.so+0xb99b1) #2 kudu::internal::RunnableAdapter::Run(kudu::master::CatalogManager*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/bind_internal.h:136:12 (libmaster.so+0x102fa9) #3 kudu::internal::InvokeHelper, void ()(kudu::master::CatalogManager*)>::MakeItSo(kudu::internal::RunnableAdapter, kudu::master::CatalogManager*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/bind_internal.h:873:14 (libmaster.so+0x102ec5) #4 kudu::internal::Invoker<1, kudu::internal::BindState, void ()(kudu::master::CatalogManager*), void ()(kudu::internal::UnretainedWrapper)>, void ()(kudu::master::CatalogManager*)>::Run(kudu::internal::BindStateBase*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/bind_internal.h:1065:12 (libmaster.so+0x102e0a) #5 kudu::Callback::Run() const /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/callback.h:396:12 (libconsensus.so+0xa6dfd) #6 kudu::ClosureRunnable::Run() /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/util/threadpool.cc:76:9 (libkudu_util.so+0x1cc9ad) #7 kudu::ThreadPool::DispatchThread() /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/util/threadpool.cc:686:22 (libkudu_util.so+0x1c86d8) #8 boost::_mfi::mf0::operator()(kudu::ThreadPool*) const /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/boost/bind/mem_fn_template.hpp:49:29 (libkudu_util.so+0x1d3649) #9 void boost::_bi::list1 >::operator(), boost::_bi::list0>(boost::_bi::type, boost::_mfi::mf0&, boost::_bi::list0&, int) /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/boost/bind/bind.hpp:259:9 (libkudu_util.so+0x1d359a) #10 boost::_bi::bind_t, boost::_bi::list1 > >::operator()() /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/boost/bind/bind.hpp:1222:16 (libkudu_util.so+0x1d3523) #11 boost::detail::function::void_function_obj_invoker0, boost::_bi::list1 > >, void>::invoke(boost::detail::function::function_buffer&) /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/boost/function/function_template.hpp:159:11 (libkudu_util.so+0x1d3319) #12 boost::function0::operator()() const /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/boost/function/function_template.hpp:770:14 (libkrpc.so+0xb6651) #13 kudu::Thread::SuperviseThread(void*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/util/thread.cc:615:3 (libkudu_util.so+0x1bfe34) Previous write of size 8 at 0x7b4c10d0 by thread T59: #0 std::__1::unique_ptr >::reset(kudu::hms::HmsCatalog*) /data/somelongdirectorytoavoidrpathissues/src/kudu/thirdparty/installed/tsan/include/c++/v1/memory:2596:20 (libmaster.so+0xb8b6f) #1 kudu::master::CatalogManager::Init(bool) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/master/catalog_manager.cc:730 (libmaster.so+0xb8b6f) #2 kudu::master::Master::InitCatalogManager() /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/master/master.cc:216:3 (libmaster.so+0x11fa5f) #3 kudu::master::Master::InitCatalogManagerTask() /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/master/master.cc:205:14 (libmaster.so+0x11f8b2) #4 kudu::internal::RunnableAdapter::Run(kudu::master::Master*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/bind_internal.h:136:12 (libmaster.so+0x124449) #5 kudu::internal::InvokeHelper, void ()(kudu::master::Master*)>::MakeItSo(kudu::internal::RunnableAdapter, kudu::master::Master*) /data/somelongdirectorytoavoidrpathissues/src/kudu/src/kudu/gutil/bind_internal.h:873:14 (libmaster.so+0x124365) #6 kudu::internal::Invoker<1, kudu::internal::BindState, void ()(kudu::master::Ma
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11819 to look at the new patch set (#2). Change subject: rpc-test: fix TestClientConnectionMetrics .. rpc-test: fix TestClientConnectionMetrics Every now and then, this test would fail with: rpc-test.cc:542: Failure Expected: (dump_resp.outbound_connections(0).outbound_queue_size()) > (0), actual: 0 vs 0 Unfortunately, the test would go one to crash (and trigger a TSAN warning) due to the lack of proper cleanup in the event of an ASSERT failure. I've fixed that in this patch. I also tried to address the root of the test flakiness (that the outbound transfer queue contains at least one element), but I couldn't find a good way to do it. Blocking the server reactor thread has no effect on client-side queuing. And we can't block the client reactor thread outright because DumpRunningRpcs runs on it. Some of this is touched on in the original code review[1] that committed the test. Having given up, I wrapped the whole thing in an ASSERT_EVENTUALLY. It's ham-fisted for sure, but it seems to work: without it, the test fails every 100-200 runs on my laptop, and with it I can't get it to fail at all. I also looped it 1000 times in TSAN mode with 8 stress threads and didn't see any failures. I don't understand the krpc subsystem very well, so if there's a better way, I'm all ears. 1. https://gerrit.cloudera.org/c/9343/ Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 --- M src/kudu/rpc/rpc-test.cc 1 file changed, 45 insertions(+), 39 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/11819/2 -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11819 to look at the new patch set (#3). Change subject: rpc-test: fix TestClientConnectionMetrics .. rpc-test: fix TestClientConnectionMetrics Every now and then, this test would fail with: rpc-test.cc:542: Failure Expected: (dump_resp.outbound_connections(0).outbound_queue_size()) > (0), actual: 0 vs 0 Unfortunately, the test would go one to crash (and trigger a TSAN warning) due to the lack of proper cleanup in the event of an ASSERT failure. I've fixed that in this patch. I also tried to address the root of the test flakiness (that the outbound transfer queue contains at least one element), but I couldn't find a good way to do it. Blocking the server reactor thread has no effect on client-side queuing. And we can't block the client reactor thread outright because DumpRunningRpcs runs on it. Some of this is touched on in the original code review[1] that committed the test. Having given up, I wrapped the whole thing in an ASSERT_EVENTUALLY. It's ham-fisted for sure, but it seems to work: without it, the test fails every 100-200 runs on my laptop, and with it I can't get it to fail at all. I also looped it 1000 times in TSAN mode with 8 stress threads and didn't see any failures. I don't understand the krpc subsystem very well, so if there's a better way, I'm all ears. 1. https://gerrit.cloudera.org/c/9343/ Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 --- M src/kudu/rpc/rpc-test.cc 1 file changed, 45 insertions(+), 42 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/11819/3 -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [rebalancer] location-aware rebalancer (part 2/n)
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11743 ) Change subject: [rebalancer] location-aware rebalancer (part 2/n) .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Ia1bdd4ee13d0522e49467b8b6e4e3a76ec560b26 Gerrit-Change-Number: 11743 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 4/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11745 ) Change subject: [rebalancer] location-aware rebalancer (part 4/n) .. Patch Set 3: Verified+1 Unrelated flake in DeleteTableITest.TestNoDeleteTombstonedTablets -- To view, visit http://gerrit.cloudera.org:8080/11745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id47183fc853573390b22ec714751adec93e0ea3a Gerrit-Change-Number: 11745 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 22:50:16 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 4/n)
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11745 ) Change subject: [rebalancer] location-aware rebalancer (part 4/n) .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: Id47183fc853573390b22ec714751adec93e0ea3a Gerrit-Change-Number: 11745 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 2/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11743 ) Change subject: [rebalancer] location-aware rebalancer (part 2/n) .. Patch Set 3: Verified+1 Unrelated flakes in: 1) org.apache.kudu.spark.kudu.DefaultSourceTest 2) org.apache.kudu.backup.TestKuduBackup -- To view, visit http://gerrit.cloudera.org:8080/11743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia1bdd4ee13d0522e49467b8b6e4e3a76ec560b26 Gerrit-Change-Number: 11743 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 22:49:42 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 5/n)
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11746 ) Change subject: [rebalancer] location-aware rebalancer (part 5/n) .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 Gerrit-Change-Number: 11746 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 2/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11743 ) Change subject: [rebalancer] location-aware rebalancer (part 2/n) .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia1bdd4ee13d0522e49467b8b6e4e3a76ec560b26 Gerrit-Change-Number: 11743 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:00:56 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 3/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11744 ) Change subject: [rebalancer] location-aware rebalancer (part 3/n) .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie89f1958238dbc28b44111038d4b82f49f824ca9 Gerrit-Change-Number: 11744 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:01:21 + Gerrit-HasComments: No
[kudu-CR] [location awareness] Add location info in ksck report
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11422 to look at the new patch set (#13). Change subject: [location_awareness] Add location info in ksck report .. [location_awareness] Add location info in ksck report This patch adds additional tablet server location info into the ksck report. Before: Tablet Server Summary UUID | Address | Status --+-+- 21b1960d8b164c869a3e600ed8bad1ea | 127.0.0.1:63000 | HEALTHY 2c55bc50306a4463aa5a935aa97caff6 | 127.0.0.1:62973 | HEALTHY a8dee6038c494b97b883d98292f7771f | 127.0.0.1:62983 | HEALTHY d3664283918a434cbc5a8e067fa7c3b0 | 127.0.0.1:62978 | HEALTHY ... After: Tablet Server Summary UUID | Address | Status | Location --+-+-+-- 21b1960d8b164c869a3e600ed8bad1ea | 127.0.0.1:63000 | HEALTHY | /foo 2c55bc50306a4463aa5a935aa97caff6 | 127.0.0.1:62973 | HEALTHY | a8dee6038c494b97b883d98292f7771f | 127.0.0.1:62983 | HEALTHY | d3664283918a434cbc5a8e067fa7c3b0 | 127.0.0.1:62978 | HEALTHY | Tablet Server Location Summary Location | Count --+- | 3 /foo | 1 ... Change-Id: Ideff2dd4975c99a1135002624de2620fb95c --- M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck.h M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/ksck_remote.h M src/kudu/tools/ksck_results.cc M src/kudu/tools/ksck_results.h M src/kudu/tools/tool.proto 10 files changed, 151 insertions(+), 30 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/11422/13 -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ideff2dd4975c99a1135002624de2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 13 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 4/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11745 ) Change subject: [rebalancer] location-aware rebalancer (part 4/n) .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id47183fc853573390b22ec714751adec93e0ea3a Gerrit-Change-Number: 11745 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:01:36 + Gerrit-HasComments: No
[kudu-CR] [location awareness] Add location info in ksck report
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11422 ) Change subject: [location_awareness] Add location info in ksck report .. Patch Set 13: (8 comments) > (3 comments) > > The location isn't getting piped through to the JSON output. You'll > also need to > > . Add the location to the KsckServerHealthSummaryPB in tool.proto. > . Set the location field in the proto in KsckServerHealthSummaryToPb > in ksck_results.cc > . Enhance CheckJsonVsServerHealthSummaries in ksck-test.cc so it > checks the locations set in the json vs the locations set in the > summaries. I see thanks! http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG@24 PS12, Line 24: Status > As for the order of fields, I think keeping the 'Status' before the 'Locati Makes sense, thanks! Done. http://gerrit.cloudera.org:8080/#/c/11422/12//COMMIT_MSG@31 PS12, Line 31: Tablet Server Location Summary > nit: maybe, add a title for this table -- 'Tablet Server Location Summary' Done http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc@1601 PS12, Line 1601: rovi > nit: why this 'Json' suffix? I see there the plain (non-json) output forma Done http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc@1620 PS12, Line 1620: " ts-id-1 | | HEAL > nit: wrap this into NO_FATALS() ? I just noticed that other tests calling this function don't have NO_FATALS() wrapped around.? http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote-test.cc@540 PS12, Line 540: > nit: substitute the location from the 'location' variable as well Done http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote.h File src/kudu/tools/ksck_remote.h: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_remote.h@95 PS12, Line 95: const > It should either by a const reference or non-const and passed-by-value with Done http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_results.cc@365 PS12, Line 365: "Status", "Location" > I added a comment on the ordering of these columns: please take a look at t Done http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck_results.cc@370 PS12, Line 370: ServerHe > nit: wrap into std::move() Done -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ideff2dd4975c99a1135002624de2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 13 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:02:36 + Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 5/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11746 ) Change subject: [rebalancer] location-aware rebalancer (part 5/n) .. Patch Set 4: Verified+1 Unrelated flakes in: * RaftConsensusParamReplicationModesITest.Test_KUDU_1735/1 * org.apache.kudu.spark.kudu.DefaultSourceTest -- To view, visit http://gerrit.cloudera.org:8080/11746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 Gerrit-Change-Number: 11746 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:04:28 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 1/n)
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11549 ) Change subject: [rebalancer] location-aware rebalancer (part 1/n) .. [rebalancer] location-aware rebalancer (part 1/n) Added logic to identify and fix placement policy violations. Also, added units tests to cover the new functionality. Change-Id: I1cdc3b57a782e6d043b1853600c97fe1f8630347 Reviewed-on: http://gerrit.cloudera.org:8080/11549 Tested-by: Kudu Jenkins Reviewed-by: Will Berkeley --- M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_results.h A src/kudu/tools/placement_policy_util-test.cc A src/kudu/tools/placement_policy_util.cc A src/kudu/tools/placement_policy_util.h M src/kudu/tools/rebalance_algo.h M src/kudu/tools/rebalancer.h 7 files changed, 1,001 insertions(+), 5 deletions(-) Approvals: Kudu Jenkins: Verified Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I1cdc3b57a782e6d043b1853600c97fe1f8630347 Gerrit-Change-Number: 11549 Gerrit-PatchSet: 9 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 6/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11747 ) Change subject: [rebalancer] location-aware rebalancer (part 6/n) .. Patch Set 4: Verified+1 Unrelated flake in org.apache.kudu.spark.kudu.DefaultSourceTest -- To view, visit http://gerrit.cloudera.org:8080/11747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I80b560d70c4d7383ee89917a359b4bb2f41bfd31 Gerrit-Change-Number: 11747 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:05:15 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 3/n)
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11744 ) Change subject: [rebalancer] location-aware rebalancer (part 3/n) .. [rebalancer] location-aware rebalancer (part 3/n) Some refactoring on the rebalancer code: * Separated rebalancer-relevant information from KsckResults into a dedicated ClusterRawInfo structure. * Added ClusterInfo structure; ClusterBalanceInfo is aggregated into the new ClusterInfo structure as a sub-field. Change-Id: Ie89f1958238dbc28b44111038d4b82f49f824ca9 Reviewed-on: http://gerrit.cloudera.org:8080/11744 Tested-by: Kudu Jenkins Reviewed-by: Will Berkeley --- M src/kudu/tools/rebalance-test.cc M src/kudu/tools/rebalance_algo-test.cc M src/kudu/tools/rebalance_algo.cc M src/kudu/tools/rebalance_algo.h M src/kudu/tools/rebalancer.cc M src/kudu/tools/rebalancer.h 6 files changed, 135 insertions(+), 108 deletions(-) Approvals: Kudu Jenkins: Verified Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11744 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ie89f1958238dbc28b44111038d4b82f49f824ca9 Gerrit-Change-Number: 11744 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] Stop using client-test-util in tools
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11821 Change subject: Stop using client-test-util in tools .. Stop using client-test-util in tools The `kudu hms` and `kudu table` tools both had actions which used functions from the client-test-util library. This meant test code was being linked into production code, which is bad. The functions used by the tool code were ScanToStrings: Used in one place in tool code, so I just wrote the KuduScanner iteration loop explicitly instead. KuduSchemaFromSchema and SchemaFromKuduSchema: Replaced (both in tests and tool code) by two static functions KuduSchema::FromSchema and KuduSchema::ToSchema. These functions are not exported. Change-Id: Ide54a0adce191a572960de678f0f7ee69eec8c2d --- M src/kudu/client/client-test-util.cc M src/kudu/client/client-test-util.h M src/kudu/client/client-unittest.cc M src/kudu/client/client.cc M src/kudu/client/scan_configuration.cc M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/integration-tests/authn_token_expire-itest.cc M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/create-table-itest.cc M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-unknown-tsk-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/test_workload.cc M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/rebalancer_tool-test.cc M src/kudu/tools/tool_action_hms.cc M src/kudu/tools/tool_action_table.cc 25 files changed, 61 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/11821/1 -- To view, visit http://gerrit.cloudera.org:8080/11821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ide54a0adce191a572960de678f0f7ee69eec8c2d Gerrit-Change-Number: 11821 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 4/n)
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11745 ) Change subject: [rebalancer] location-aware rebalancer (part 4/n) .. [rebalancer] location-aware rebalancer (part 4/n) Refactored Rebalancer and Runner classes, separating common base for a runner of the rebalancing process. Change-Id: Id47183fc853573390b22ec714751adec93e0ea3a Reviewed-on: http://gerrit.cloudera.org:8080/11745 Tested-by: Alexey Serbin Reviewed-by: Will Berkeley --- M src/kudu/tools/rebalancer.cc M src/kudu/tools/rebalancer.h 2 files changed, 487 insertions(+), 374 deletions(-) Approvals: Alexey Serbin: Verified Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id47183fc853573390b22ec714751adec93e0ea3a Gerrit-Change-Number: 11745 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 6/n)
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11747 ) Change subject: [rebalancer] location-aware rebalancer (part 6/n) .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I80b560d70c4d7383ee89917a359b4bb2f41bfd31 Gerrit-Change-Number: 11747 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 2/n)
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11743 ) Change subject: [rebalancer] location-aware rebalancer (part 2/n) .. [rebalancer] location-aware rebalancer (part 2/n) Small refactoring on tools::Rebalancer: renamed Rebalancer::ResetKsck() into Rebalancer::RefreshKsckResults() and made the actual ksck utility run as a part of the renamed method. Change-Id: Ia1bdd4ee13d0522e49467b8b6e4e3a76ec560b26 Reviewed-on: http://gerrit.cloudera.org:8080/11743 Tested-by: Alexey Serbin Reviewed-by: Will Berkeley --- M src/kudu/tools/rebalancer.cc M src/kudu/tools/rebalancer.h 2 files changed, 13 insertions(+), 21 deletions(-) Approvals: Alexey Serbin: Verified Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11743 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia1bdd4ee13d0522e49467b8b6e4e3a76ec560b26 Gerrit-Change-Number: 11743 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 1/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11549 ) Change subject: [rebalancer] location-aware rebalancer (part 1/n) .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11549 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1cdc3b57a782e6d043b1853600c97fe1f8630347 Gerrit-Change-Number: 11549 Gerrit-PatchSet: 8 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:00:43 + Gerrit-HasComments: No
[kudu-CR] Stop using client-test-util in tools
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11821 ) Change subject: Stop using client-test-util in tools .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11821/1/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/11821/1/src/kudu/client/schema.h@579 PS1, Line 579: static Schema ToSchema(const KuduSchema& kudu_schema) KUDU_NO_EXPORT; > warning: function 'kudu::client::KuduSchema::ToSchema' has a definition wit Done -- To view, visit http://gerrit.cloudera.org:8080/11821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide54a0adce191a572960de678f0f7ee69eec8c2d Gerrit-Change-Number: 11821 Gerrit-PatchSet: 1 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:16:32 + Gerrit-HasComments: Yes
[kudu-CR] Stop using client-test-util in tools
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11821 to look at the new patch set (#2). Change subject: Stop using client-test-util in tools .. Stop using client-test-util in tools The `kudu hms` and `kudu table` tools both had actions which used functions from the client-test-util library. This meant test code was being linked into production code, which is bad. The functions used by the tool code were ScanToStrings: Used in one place in tool code, so I just wrote the KuduScanner iteration loop explicitly instead. KuduSchemaFromSchema and SchemaFromKuduSchema: Replaced (both in tests and tool code) by two static functions KuduSchema::FromSchema and KuduSchema::ToSchema. These functions are not exported. Change-Id: Ide54a0adce191a572960de678f0f7ee69eec8c2d --- M src/kudu/client/client-test-util.cc M src/kudu/client/client-test-util.h M src/kudu/client/client-unittest.cc M src/kudu/client/client.cc M src/kudu/client/scan_configuration.cc M src/kudu/client/schema.cc M src/kudu/client/schema.h M src/kudu/integration-tests/authn_token_expire-itest.cc M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/create-table-itest.cc M src/kudu/integration-tests/delete_table-itest.cc M src/kudu/integration-tests/fuzz-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/security-itest.cc M src/kudu/integration-tests/security-unknown-tsk-itest.cc M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/integration-tests/test_workload.cc M src/kudu/integration-tests/ts_itest-base.cc M src/kudu/tools/CMakeLists.txt M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/rebalancer_tool-test.cc M src/kudu/tools/tool_action_hms.cc M src/kudu/tools/tool_action_table.cc 25 files changed, 61 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/11821/2 -- To view, visit http://gerrit.cloudera.org:8080/11821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ide54a0adce191a572960de678f0f7ee69eec8c2d Gerrit-Change-Number: 11821 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [rebalancer] location-aware rebalancer (part 5/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11746 ) Change subject: [rebalancer] location-aware rebalancer (part 5/n) .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 Gerrit-Change-Number: 11746 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:17:26 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 6/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11747 ) Change subject: [rebalancer] location-aware rebalancer (part 6/n) .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I80b560d70c4d7383ee89917a359b4bb2f41bfd31 Gerrit-Change-Number: 11747 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:17:35 + Gerrit-HasComments: No
[kudu-CR] [location awareness] Add location info in ksck report
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11422 to look at the new patch set (#14). Change subject: [location_awareness] Add location info in ksck report .. [location_awareness] Add location info in ksck report This patch adds additional tablet server location info into the ksck report. Before: Tablet Server Summary UUID | Address | Status --+-+- 21b1960d8b164c869a3e600ed8bad1ea | 127.0.0.1:63000 | HEALTHY 2c55bc50306a4463aa5a935aa97caff6 | 127.0.0.1:62973 | HEALTHY a8dee6038c494b97b883d98292f7771f | 127.0.0.1:62983 | HEALTHY d3664283918a434cbc5a8e067fa7c3b0 | 127.0.0.1:62978 | HEALTHY ... After: Tablet Server Summary UUID | Address | Status | Location --+-+-+-- 21b1960d8b164c869a3e600ed8bad1ea | 127.0.0.1:63000 | HEALTHY | /foo 2c55bc50306a4463aa5a935aa97caff6 | 127.0.0.1:62973 | HEALTHY | a8dee6038c494b97b883d98292f7771f | 127.0.0.1:62983 | HEALTHY | d3664283918a434cbc5a8e067fa7c3b0 | 127.0.0.1:62978 | HEALTHY | Tablet Server Location Summary Location | Count --+- | 3 /foo | 1 ... Change-Id: Ideff2dd4975c99a1135002624de2620fb95c --- M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck.h M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/ksck_remote.h M src/kudu/tools/ksck_results.cc M src/kudu/tools/ksck_results.h M src/kudu/tools/tool.proto 10 files changed, 149 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/11422/14 -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ideff2dd4975c99a1135002624de2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 14 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 7/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11748 ) Change subject: [rebalancer] location-aware rebalancer (part 7/n) .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11748 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9ace790aad1c1a4605ef90f6df2104f4a228a5b5 Gerrit-Change-Number: 11748 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:17:46 + Gerrit-HasComments: No
[kudu-CR] [rebalancer] location-aware rebalancer (part 8/n)
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11761 ) Change subject: [rebalancer] location-aware rebalancer (part 8/n) .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util.cc File src/kudu/tools/placement_policy_util.cc: http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util.cc@81 PS1, Line 81: > Well, I think this case is a separate one. Yeah, my bad. -- To view, visit http://gerrit.cloudera.org:8080/11761 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f8831d254b2ca0d9a12e0ffbc336a59c3c5c8de Gerrit-Change-Number: 11761 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:19:26 + Gerrit-HasComments: Yes
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Hello Alexey Serbin, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11819 to look at the new patch set (#4). Change subject: rpc-test: fix TestClientConnectionMetrics .. rpc-test: fix TestClientConnectionMetrics Every now and then, this test would fail with: rpc-test.cc:542: Failure Expected: (dump_resp.outbound_connections(0).outbound_queue_size()) > (0), actual: 0 vs 0 Unfortunately, the test would go one to crash (and trigger a TSAN warning) due to the lack of proper cleanup in the event of an ASSERT failure. I've fixed that in this patch. I also tried to address the root of the test flakiness (that the outbound transfer queue contains at least one element), but I couldn't find a good way to do it. Blocking the server reactor thread has no effect on client-side queuing. And we can't block the client reactor thread outright because DumpRunningRpcs runs on it. Some of this is touched on in the original code review[1] that committed the test. Having given up, I wrapped the whole thing in an ASSERT_EVENTUALLY. It's ham-fisted for sure, but it seems to work: without it, the test fails every 100-200 runs on my laptop, and with it I can't get it to fail at all. I also looped it 1000 times in TSAN mode with 8 stress threads and didn't see any failures. I don't understand the krpc subsystem very well, so if there's a better way, I'm all ears. 1. https://gerrit.cloudera.org/c/9343/ Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 --- M src/kudu/rpc/rpc-test.cc 1 file changed, 45 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/11819/4 -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] catalog manager: fix a TSAN race
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11818 ) Change subject: catalog_manager: fix a TSAN race .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/11818/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11818/1/src/kudu/master/catalog_manager.cc@735 PS1, Line 735:std::lock_guard leader_lock_guard(leader_lock_); > Yes, this was intentional. I don't believe background_tasks.reset() needs a Ah, I see; thank you for the explanation. -- To view, visit http://gerrit.cloudera.org:8080/11818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I090a832b7fb25d8cb1e770c025048f73ac997eac Gerrit-Change-Number: 11818 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 29 Oct 2018 23:26:23 + Gerrit-HasComments: Yes
[kudu-CR] [rebalancer] location-aware rebalancer (part 5/n)
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11746 ) Change subject: [rebalancer] location-aware rebalancer (part 5/n) .. [rebalancer] location-aware rebalancer (part 5/n) Added LocationBalancingAlgo and corresponding units tests. Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 Reviewed-on: http://gerrit.cloudera.org:8080/11746 Tested-by: Alexey Serbin Reviewed-by: Will Berkeley --- M src/kudu/tools/rebalance_algo-test.cc M src/kudu/tools/rebalance_algo.cc M src/kudu/tools/rebalance_algo.h 3 files changed, 623 insertions(+), 4 deletions(-) Approvals: Alexey Serbin: Verified Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11746 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I78446fec8b8f80b7c6112bdd9d53f3dbf506 Gerrit-Change-Number: 11746 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 6/n)
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11747 ) Change subject: [rebalancer] location-aware rebalancer (part 6/n) .. [rebalancer] location-aware rebalancer (part 6/n) Added SetReplace() and CheckCompleteReplace() auxiliary fuctions. A follow-up patch will start using those. Change-Id: I80b560d70c4d7383ee89917a359b4bb2f41bfd31 Reviewed-on: http://gerrit.cloudera.org:8080/11747 Tested-by: Alexey Serbin Reviewed-by: Will Berkeley --- M src/kudu/tools/tool_replica_util.cc M src/kudu/tools/tool_replica_util.h 2 files changed, 157 insertions(+), 0 deletions(-) Approvals: Alexey Serbin: Verified Will Berkeley: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/11747 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I80b560d70c4d7383ee89917a359b4bb2f41bfd31 Gerrit-Change-Number: 11747 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Will Berkeley
[kudu-CR] [rebalancer] location-aware rebalancer (part 8/n)
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11761 ) Change subject: [rebalancer] location-aware rebalancer (part 8/n) .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util.cc File src/kudu/tools/placement_policy_util.cc: http://gerrit.cloudera.org:8080/#/c/11761/1/src/kudu/tools/placement_policy_util.cc@81 PS1, Line 81: == > Yeah, my bad. NP, that was not obvious. I think that means I need to add a comment with the explanation. -- To view, visit http://gerrit.cloudera.org:8080/11761 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f8831d254b2ca0d9a12e0ffbc336a59c3c5c8de Gerrit-Change-Number: 11761 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:28:53 + Gerrit-HasComments: Yes
[kudu-CR] Stop using client-test-util in tools
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11821 ) Change subject: Stop using client-test-util in tools .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/11821/2/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/11821/2/src/kudu/client/schema.h@577 PS2, Line 577: // Convert a KuduSchema to or from an internal Schema. This probably merits a more Doxygen-friendly syntax. See client.h for some examples of non-exported functions with "private API" Doxygen syntax. -- To view, visit http://gerrit.cloudera.org:8080/11821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide54a0adce191a572960de678f0f7ee69eec8c2d Gerrit-Change-Number: 11821 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:35:59 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add location info in ksck report
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11422 ) Change subject: [location_awareness] Add location info in ksck report .. Patch Set 14: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc@1620 PS12, Line 1620: " ts-id-1 | | HEAL > I just noticed that other tests calling this function don't have NO_FATALS( That just means an error from this function would not be caught as soon as it happened in those tests: it could make troubleshooting a bit cumbersome if an error happened. http://gerrit.cloudera.org:8080/#/c/11422/14/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/14/src/kudu/tools/ksck_remote-test.cc@280 PS14, Line 280: LOG(INFO) << err_stream_.str(); nit: is it really needed? -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ideff2dd4975c99a1135002624de2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 14 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 29 Oct 2018 23:37:40 + Gerrit-HasComments: Yes
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11819 ) Change subject: rpc-test: fix TestClientConnectionMetrics .. Patch Set 4: Verified+1 Overriding Jenkins, unrelated test failure. -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Oct 2018 00:08:26 + Gerrit-HasComments: No
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/11819 ) Change subject: rpc-test: fix TestClientConnectionMetrics .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11819 ) Change subject: rpc-test: fix TestClientConnectionMetrics .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Oct 2018 00:23:32 + Gerrit-HasComments: No
[kudu-CR] Stop using client-test-util in tools
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11821 ) Change subject: Stop using client-test-util in tools .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/11821/2/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/11821/2/src/kudu/client/schema.h@577 PS2, Line 577: // Convert a KuduSchema to or from an internal Schema. > This probably merits a more Doxygen-friendly syntax. See client.h for some +1 http://gerrit.cloudera.org:8080/#/c/11821/2/src/kudu/client/schema.h@578 PS2, Line 578: static KuduSchema FromSchema(const Schema& schema) KUDU_NO_EXPORT; : static Schema ToSchema(const KuduSchema& kudu_schema) KUDU_NO_EXPORT; Could you add doxygen-style documentation for those functions and put that under the @cond PRIVATE_API condition? That helps to get rid of doxygen warnings while generating the API docs. An example: https://github.com/apache/kudu/blob/master/src/kudu/client/client.h#L342 -- To view, visit http://gerrit.cloudera.org:8080/11821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide54a0adce191a572960de678f0f7ee69eec8c2d Gerrit-Change-Number: 11821 Gerrit-PatchSet: 2 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 30 Oct 2018 00:24:39 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add location info in ksck report
Hello Will Berkeley, Tidy Bot, Alexey Serbin, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11422 to look at the new patch set (#15). Change subject: [location_awareness] Add location info in ksck report .. [location_awareness] Add location info in ksck report This patch adds additional tablet server location info into the ksck report. Before: Tablet Server Summary UUID | Address | Status --+-+- 21b1960d8b164c869a3e600ed8bad1ea | 127.0.0.1:63000 | HEALTHY 2c55bc50306a4463aa5a935aa97caff6 | 127.0.0.1:62973 | HEALTHY a8dee6038c494b97b883d98292f7771f | 127.0.0.1:62983 | HEALTHY d3664283918a434cbc5a8e067fa7c3b0 | 127.0.0.1:62978 | HEALTHY ... After: Tablet Server Summary UUID | Address | Status | Location --+-+-+-- 21b1960d8b164c869a3e600ed8bad1ea | 127.0.0.1:63000 | HEALTHY | /foo 2c55bc50306a4463aa5a935aa97caff6 | 127.0.0.1:62973 | HEALTHY | a8dee6038c494b97b883d98292f7771f | 127.0.0.1:62983 | HEALTHY | d3664283918a434cbc5a8e067fa7c3b0 | 127.0.0.1:62978 | HEALTHY | Tablet Server Location Summary Location | Count --+- | 3 /foo | 1 ... Change-Id: Ideff2dd4975c99a1135002624de2620fb95c --- M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck-test.cc M src/kudu/tools/ksck.cc M src/kudu/tools/ksck.h M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/ksck_remote.cc M src/kudu/tools/ksck_remote.h M src/kudu/tools/ksck_results.cc M src/kudu/tools/ksck_results.h M src/kudu/tools/tool.proto 10 files changed, 148 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/11422/15 -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ideff2dd4975c99a1135002624de2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 15 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley
[kudu-CR] [location awareness] Add location info in ksck report
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/11422 ) Change subject: [location_awareness] Add location info in ksck report .. Patch Set 12: (2 comments) http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/12/src/kudu/tools/ksck-test.cc@1620 PS12, Line 1620: CheckJsonStringVsKsckResults > That just means an error from this function would not be caught as soon as Done http://gerrit.cloudera.org:8080/#/c/11422/14/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/11422/14/src/kudu/tools/ksck_remote-test.cc@280 PS14, Line 280: string match_string = "Error fr > nit: is it really needed? opps, forgot to remove -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ideff2dd4975c99a1135002624de2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 12 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 30 Oct 2018 00:37:15 + Gerrit-HasComments: Yes
[kudu-CR] catalog manager: fix a TSAN race
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11818 ) Change subject: catalog_manager: fix a TSAN race .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11818 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I090a832b7fb25d8cb1e770c025048f73ac997eac Gerrit-Change-Number: 11818 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Oct 2018 00:54:07 + Gerrit-HasComments: No
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11819 ) Change subject: rpc-test: fix TestClientConnectionMetrics .. Patch Set 4: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/11819/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11819/4//COMMIT_MSG@14 PS4, Line 14: one nit: on -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Oct 2018 01:23:11 + Gerrit-HasComments: Yes
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Hello Alexey Serbin, Andrew Wong, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11819 to look at the new patch set (#5). Change subject: rpc-test: fix TestClientConnectionMetrics .. rpc-test: fix TestClientConnectionMetrics Every now and then, this test would fail with: rpc-test.cc:542: Failure Expected: (dump_resp.outbound_connections(0).outbound_queue_size()) > (0), actual: 0 vs 0 Unfortunately, the test would go on to crash (and trigger a TSAN warning) due to the lack of proper cleanup in the event of an ASSERT failure. I've fixed that in this patch. I also tried to address the root of the test flakiness (that the outbound transfer queue contains at least one element), but I couldn't find a good way to do it. Blocking the server reactor thread has no effect on client-side queuing. And we can't block the client reactor thread outright because DumpRunningRpcs runs on it. Some of this is touched on in the original code review[1] that committed the test. Having given up, I wrapped the whole thing in an ASSERT_EVENTUALLY. It's ham-fisted for sure, but it seems to work: without it, the test fails every 100-200 runs on my laptop, and with it I can't get it to fail at all. I also looped it 1000 times in TSAN mode with 8 stress threads and didn't see any failures. I don't understand the krpc subsystem very well, so if there's a better way, I'm all ears. 1. https://gerrit.cloudera.org/c/9343/ Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 --- M src/kudu/rpc/rpc-test.cc 1 file changed, 45 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/11819/5 -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11819 ) Change subject: rpc-test: fix TestClientConnectionMetrics .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/11819/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11819/4//COMMIT_MSG@14 PS4, Line 14: on > nit: on Done -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Oct 2018 03:41:51 + Gerrit-HasComments: Yes
[kudu-CR] [location awareness] Add location info in ksck report
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11422 ) Change subject: [location_awareness] Add location info in ksck report .. Patch Set 15: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/11422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ideff2dd4975c99a1135002624de2620fb95c Gerrit-Change-Number: 11422 Gerrit-PatchSet: 15 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 30 Oct 2018 03:58:43 + Gerrit-HasComments: No
[kudu-CR] rpc-test: fix TestClientConnectionMetrics
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11819 ) Change subject: rpc-test: fix TestClientConnectionMetrics .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/11819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9c565b80bdca435d18787c7df0ec992728363980 Gerrit-Change-Number: 11819 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Oct 2018 03:58:02 + Gerrit-HasComments: No
[kudu-CR] deltas: fuzz tests for delta file and dms
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11140 ) Change subject: deltas: fuzz tests for delta file and dms .. Patch Set 15: (8 comments) http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h File src/kudu/tablet/tablet-test-util.h: http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@354 PS15, Line 354:T::kTag == REDO, :std::less, :std::greater>::type; > nit: unusual indentation Done http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@454 PS15, Line 454: // Transforms and writes deltas into 'deltas', a vector of "delta lists", each > please add a description of the semantics of this method (what it's doing / I don't think your suggestion is adding more information than already exists. "Collect all mutations" is implied by the method name. And "relevant to a scan at timestamp ts" is already mentioned in the second paragraph, in "Deltas not relevant to 'ts' are skipped". http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@467 PS15, Line 467: continue > break? Done http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@750 PS15, Line 750: // Runs a fuzz test by generating a fairly random DeltaIterator, using it to > Clarify what component is being tested here Done http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@772 PS15, Line 772: inclusive snapshot > what does this mean? the prng->Uniform() logic below treats .second as excl Yeah this wasn't clear. This is referring to how when i == kNumScans, the snapshot we use is "totally inclusive", which is an ambiguous way of saying "a snapshot for which all deltas are relevant". http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.h@810 PS15, Line 810: - 1 > nit: no need for this -1 because Random::Uniform() returns values from [0 . Done http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.cc File src/kudu/tablet/tablet-test-util.cc: http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.cc@26 PS15, Line 26: to_include > ts; > nit: maybe it's just me, but I usually think of this as ts < to_include, be Done http://gerrit.cloudera.org:8080/#/c/11140/15/src/kudu/tablet/tablet-test-util.cc@32 PS15, Line 32: to_include <= ts > same here, I usually think of this as ts >= to_include Done -- To view, visit http://gerrit.cloudera.org:8080/11140 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I539ff0f2055cf398a42efb824238188230e25516 Gerrit-Change-Number: 11140 Gerrit-PatchSet: 15 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 30 Oct 2018 04:00:23 + Gerrit-HasComments: Yes
