[kudu-CR] [WIP] [tools] ksck improvements [6/n]: Refactor printing
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10151 to look at the new patch set (#7). Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing .. [WIP] [tools] ksck improvements [6/n]: Refactor printing This patch significantly refactors printing in ksck. It separates where checks are done from where results are printed. This separation will make it easier to provide machine-readable output and create tools based on the results of ksck checks, like auto-repair. There's also a couple of bonus changes: - Simplifies running all ksck checks and printing the results, as done by the ksck tool and ClusterVerifier. - Removes the --consensus flag. ksck is far less useful without it and it doesn't seem like having it brings any benefit. - Add the changes in https://gerrit.cloudera.org/#/c/10054/, which were about to be committed but would've needed to be redone a bit to fit with this refactor. Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 --- M src/kudu/integration-tests/cluster_verifier.cc 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 A src/kudu/tools/ksck_results.cc A src/kudu/tools/ksck_results.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 1,204 insertions(+), 887 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/10151/7 -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 7 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [WIP] [tools] ksck improvements [6/n]: Refactor printing
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10151 ) Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing .. Patch Set 6: (15 comments) http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG@17 PS5, Line 17: - Removes the --consensus flag. ksck is far less useful without it and : it doesn't seem like having it brings any benefit. > Any concerns about that in terms of backward compatibility? I don't think we make guarantees on backwards compat for tools. Also, I don't think anyone besides me ever used that flag. The good info comes from the consensus state :) I see the point about removing it separately but if I make it a patch prior to this one I'm spending time working in the "old way" and have to rejigger the patch to match those changes. Seems wasteful. If I make it a patch after this one I have to redo pieces of this patch to re-introduce it, just so I can take it out. Ergo, unless you have a big problem with keeping it part of this patch, I'd rather leave it in. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/integration-tests/cluster_verifier.cc File src/kudu/integration-tests/cluster_verifier.cc: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/integration-tests/cluster_verifier.cc@112 PS5, Line 112: RunAndPrintResults > Would it make sense to separate that in two methods: Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@845 PS5, Line 845: /*underreplicated_tables=*/ 3, : /*consensus_mismatch_tables=*/ 0, : /*unavailable_tables=*/ 0)); : } > Here an in other places: Done. Plus some changes due to how I wrote RunAndPrintResults(). http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@858 PS5, Line 858: // Now engineer a consensus conflict. : const shared_ptr& ts = FindOrDie(cluster_->tablet_servers_, "ts-id-1"); : auto& cstate > nit: why not just Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@878 PS5, Line 878: ASSERT_STR_CONTAINS(err_stream_.str(), : ExpectedKsckTableSummary("test", : > ASSERT_OK(RunKsck()) ? Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h File src/kudu/tools/ksck_results.h: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@31 PS5, Line 31: "kudu/util/status.h" > why angle brackets instead of quotes? Oops. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@126 PS5, Line 126: const char* const ServerHealthToString(KsckServerHealth sh); : > It would be nice to mention what higher score value means, i.e. if server A Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@133 PS5, Line 133: f a ser > What if server has multiple RPC endpoints? AFAIK we routinely don't handle this situation in our code. I don't think we support it overall even though some places are set up for it. What would end up here is what the master reports as the RPC endpoint. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@257 PS5, Line 257: ch table and tablet. > nit: why not constant reference? Oops. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@88 PS5, Line 88: CHECK(uuid_label_mapping); > static? It seems this function can be entered multiple times during kudu C First point: It's in an anonymous namespace, so it's functionally equivalent to a static function. Second: Done. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@124 PS5, Line 124: switch (cr) { > nit: maybe, return 'const char* const' instead? Done. Also done for function above. http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@177 PS5, Line 177: // First, report on the masters and master tablet. > nit: add a stop Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@268 PS5, Line 268: } > nit: two extra spaces Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@269 PS5, Line 269: uid : server_uuids) { : const string label(1, FindOrDie(replica_labels, uuid)); > consider adding replication factor, as it's done in https://gerrit.cloudera Done http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@273 PS5, Line 273:
[kudu-CR] [WIP] [tools] ksck improvements [6/n]: Refactor printing
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10151 to look at the new patch set (#6). Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing .. [WIP] [tools] ksck improvements [6/n]: Refactor printing This patch significantly refactors printing in ksck. It separates where checks are done from where results are printed. This separation will make it easier to provide machine-readable output and create tools based on the results of ksck checks, like auto-repair. There's also a couple of bonus changes: - Simplifies running all ksck checks and printing the results, as done by the ksck tool and ClusterVerifier. - Removes the --consensus flag. ksck is far less useful without it and it doesn't seem like having it brings any benefit. - Add the changes in https://gerrit.cloudera.org/#/c/10054/, which were about to be committed but would've needed to be redone a bit to fit with this refactor. Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 --- M src/kudu/integration-tests/cluster_verifier.cc 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 A src/kudu/tools/ksck_results.cc A src/kudu/tools/ksck_results.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 1,206 insertions(+), 887 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/10151/6 -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 6 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docs] Add a navigation link to the scaling guide
Grant Henke has removed a vote on this change. Change subject: [docs] Add a navigation link to the scaling guide .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/10181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I3119d39b14c682fa40022ea5e5c199df15a033b2 Gerrit-Change-Number: 10181 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docs] Add a navigation link to the scaling guide
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10181 ) Change subject: [docs] Add a navigation link to the scaling guide .. [docs] Add a navigation link to the scaling guide Change-Id: I3119d39b14c682fa40022ea5e5c199df15a033b2 Reviewed-on: http://gerrit.cloudera.org:8080/10181 Reviewed-by: Will Berkeley Tested-by: Grant Henke --- M docs/support/jekyll-templates/document.html.erb 1 file changed, 1 insertion(+), 0 deletions(-) Approvals: Will Berkeley: Looks good to me, approved Grant Henke: Verified -- To view, visit http://gerrit.cloudera.org:8080/10181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3119d39b14c682fa40022ea5e5c199df15a033b2 Gerrit-Change-Number: 10181 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] [docs] Add a navigation link to the scaling guide
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10181 ) Change subject: [docs] Add a navigation link to the scaling guide .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/10181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3119d39b14c682fa40022ea5e5c199df15a033b2 Gerrit-Change-Number: 10181 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Wed, 25 Apr 2018 02:57:40 + Gerrit-HasComments: No
[kudu-CR] KUDU-2416: Fix PartialRow.setMin and add a unit test
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10185 Change subject: KUDU-2416: Fix PartialRow.setMin and add a unit test .. KUDU-2416: Fix PartialRow.setMin and add a unit test * Fixes a missing break after the decimal type that could lead to IllegalArgumentExceptions. * Fixes INTEGER.MIN_VALUE being used for LONG colums that could lead to incorrect partition prunning. * Adds a unit test for all types. Change-Id: I510743385cc567a8443efc8b0c9f5b641cf04c31 --- M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestPartialRow.java 2 files changed, 22 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/10185/1 -- To view, visit http://gerrit.cloudera.org:8080/10185 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I510743385cc567a8443efc8b0c9f5b641cf04c31 Gerrit-Change-Number: 10185 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] Re-enable IWYU checks on HMS module
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10102 ) Change subject: Re-enable IWYU checks on HMS module .. Re-enable IWYU checks on HMS module IWYU previously had some issues with the HMS module[1], probably due to the Thrift generated classes. Now that IWYU has been upgraded[2] this is an attempt to re-enable the IWYU checks. This commit also includes some associated fixups. [1]: https://github.com/apache/kudu/commit/c09a6296181ecbb7d91687536f70fdbaac9fff5e [2]: https://github.com/apache/kudu/commit/bba339b11795d84da9c9d1473366ca561730c141 Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Reviewed-on: http://gerrit.cloudera.org:8080/10102 Tested-by: Alexey Serbin Reviewed-by: Alexey Serbin --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.h 8 files changed, 43 insertions(+), 37 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/10102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Gerrit-Change-Number: 10102 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Re-enable IWYU checks on HMS module
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10102 ) Change subject: Re-enable IWYU checks on HMS module .. Patch Set 3: Verified+1 The failure is due to a flakiness in ConsensusPeerHealthStatusITest.TestPeerHealthStatusTransitions (ASAN configuration). I suspect the problem is that the replica could not get behind WAL log gc in 30 seconds. -- To view, visit http://gerrit.cloudera.org:8080/10102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Gerrit-Change-Number: 10102 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Apr 2018 02:29:20 + Gerrit-HasComments: No
[kudu-CR] Re-enable IWYU checks on HMS module
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10102 ) Change subject: Re-enable IWYU checks on HMS module .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Gerrit-Change-Number: 10102 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Apr 2018 02:29:23 + Gerrit-HasComments: No
[kudu-CR] Re-enable IWYU checks on HMS module
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/10102 ) Change subject: Re-enable IWYU checks on HMS module .. Removed reviewer Kudu Jenkins with the following votes: * Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/10102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteReviewer Gerrit-Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Gerrit-Change-Number: 10102 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] WIP: java: enable error-prone for java builds
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/4425 ) Change subject: WIP: java: enable error-prone for java builds .. Patch Set 3: (3 comments) How does error-prone compare to findbugs/spotbugs? http://gerrit.cloudera.org:8080/#/c/4425/3/java/build.gradle File java/build.gradle: http://gerrit.cloudera.org:8080/#/c/4425/3/java/build.gradle@24 PS3, Line 24: id 'net.ltgt.errorprone' version '0.0.13' apply false I have been putting all of these static analysis checks into quality.gradle http://gerrit.cloudera.org:8080/#/c/4425/3/java/build.gradle@48 PS3, Line 48: apply plugin: 'java' Any reason you changed this to single quotes? http://gerrit.cloudera.org:8080/#/c/4425/3/java/build.gradle@51 PS3, Line 51: apply from: "$rootDir/gradle/errorprone.gradle" Did you add this file? I have been putting all of these static analysis checks into quality.gradle -- To view, visit http://gerrit.cloudera.org:8080/4425 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d Gerrit-Change-Number: 4425 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Wed, 25 Apr 2018 01:31:18 + Gerrit-HasComments: Yes
[kudu-CR] Re-enable IWYU checks on HMS module
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10102 ) Change subject: Re-enable IWYU checks on HMS module .. Patch Set 3: > > > Rebased. I'm a little confused, because I'm not getting iwyu > > > errors before _or_ after applying this patch on an internal > build > > > machine. We'll see what gerrit says. I think the code > cleanups > > in > > > this patch are worth landing. > > > > Yeah, I think the reason you didn't see errors on your local > build > > machine is that most likely Thrift-related files were already > > generated when you ran the 'iwyu' target. However, in case of > > Jenkins slaves for pre-commit builds, the IWYU configuration just > > run 'make iwyu' target before as is, without building the rest of > > the sources. > > I just verified on ve0518 with pre-72847ab (i.e. pre-based): if > first building everything (i.e. just run 'make') and then run 'make > iwyu', there were no warnings from IWYU. However, if doing 'make > iwyu' on the clean build directory, there were nonsense-style > warnings from IWYU. And yes, with post-72847ab version the problem described above is gone (at least at ve0518): if running 'make iwyu' target on the freshly configured build directory, IWYU does not report any warnings. -- To view, visit http://gerrit.cloudera.org:8080/10102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Gerrit-Change-Number: 10102 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Apr 2018 01:12:23 + Gerrit-HasComments: No
[kudu-CR] Re-enable IWYU checks on HMS module
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10102 ) Change subject: Re-enable IWYU checks on HMS module .. Patch Set 3: > > Rebased. I'm a little confused, because I'm not getting iwyu > > errors before _or_ after applying this patch on an internal build > > machine. We'll see what gerrit says. I think the code cleanups > in > > this patch are worth landing. > > Yeah, I think the reason you didn't see errors on your local build > machine is that most likely Thrift-related files were already > generated when you ran the 'iwyu' target. However, in case of > Jenkins slaves for pre-commit builds, the IWYU configuration just > run 'make iwyu' target before as is, without building the rest of > the sources. I just verified on ve0518 with pre-72847ab (i.e. pre-based): if first building everything (i.e. just run 'make') and then run 'make iwyu', there were no warnings from IWYU. However, if doing 'make iwyu' on the clean build directory, there were nonsense-style warnings from IWYU. -- To view, visit http://gerrit.cloudera.org:8080/10102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Gerrit-Change-Number: 10102 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Apr 2018 01:07:17 + Gerrit-HasComments: No
[kudu-CR] WIP: java: enable error-prone for java builds
Hello Dan Burkert, Jean-Daniel Cryans, Kudu Jenkins, Grant Henke, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/4425 to look at the new patch set (#3). Change subject: WIP: java: enable error-prone for java builds .. WIP: java: enable error-prone for java builds error-prone (http://errorprone.info/) is a compiler extension by Google which checks for a bunch of common Java errors. One of the most useful features is that it verifies JCIP annotations like GuardedBy. This includes some fixes to get it to pass. We should break these fixes out into separate commits. WIP: not quite done fixing all errors yet, and likely worth breaing out some fixes as above Change-Id: Iebafbb307904875490426dd699ab1f55e21d284d --- M java/build.gradle M java/gradle/dependencies.gradle M java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/ImportParquet.java M java/kudu-client-tools/src/main/java/org/apache/kudu/mapreduce/tools/IntegrationTestBigLinkedList.java M java/kudu-client/src/main/java/org/apache/kudu/Type.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java M java/kudu-client/src/main/java/org/apache/kudu/client/Batch.java M java/kudu-client/src/main/java/org/apache/kudu/client/Bytes.java M java/kudu-client/src/main/java/org/apache/kudu/client/ColumnRangePredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToMasterRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java M java/kudu-client/src/main/java/org/apache/kudu/client/CreateTableOptions.java M java/kudu-client/src/main/java/org/apache/kudu/client/DeadlineTracker.java M java/kudu-client/src/main/java/org/apache/kudu/client/ExternalConsistencyMode.java M java/kudu-client/src/main/java/org/apache/kudu/client/IsAlterTableDoneRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/IsCreateTableDoneRequest.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduPredicate.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanner.java M java/kudu-client/src/main/java/org/apache/kudu/client/LocatedTablet.java M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/PartialRow.java M java/kudu-client/src/main/java/org/apache/kudu/client/PartitionPruner.java M java/kudu-client/src/main/java/org/apache/kudu/client/PleaseThrottleException.java M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java M java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java M java/kudu-client/src/main/java/org/apache/kudu/client/RowError.java M java/kudu-client/src/main/java/org/apache/kudu/client/RowResult.java M java/kudu-client/src/main/java/org/apache/kudu/client/RowResultIterator.java M java/kudu-client/src/main/java/org/apache/kudu/client/RpcProxy.java M java/kudu-client/src/main/java/org/apache/kudu/client/RpcTraceFrame.java M java/kudu-client/src/main/java/org/apache/kudu/client/Statistics.java M java/kudu-client/src/main/java/org/apache/kudu/client/Status.java M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java M java/kudu-client/src/main/java/org/apache/kudu/util/SecurityUtil.java M java/kudu-client/src/main/java/org/apache/kudu/util/Slice.java M java/kudu-client/src/main/java/org/apache/kudu/util/Slices.java M java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestAuthnTokenReacquire.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestBytes.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduPredicate.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestRowResult.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestTestUtils.java M java/kudu-client/src/test/java/org/apache/kudu/util/TestAsyncUtil.java M java/kudu-flume-sink/src/main/java/org/apache/kudu/flume/sink/KuduSin
[kudu-CR] Re-enable IWYU checks on HMS module
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10102 ) Change subject: Re-enable IWYU checks on HMS module .. Patch Set 3: > Rebased. I'm a little confused, because I'm not getting iwyu > errors before _or_ after applying this patch on an internal build > machine. We'll see what gerrit says. I think the code cleanups in > this patch are worth landing. Yeah, I think the reason you didn't see errors on your local build machine is that most likely Thrift-related files were already generated when you ran the 'iwyu' target. However, in case of Jenkins slaves for pre-commit builds, the IWYU configuration just run 'make iwyu' target before as is, without building the rest of the sources. -- To view, visit http://gerrit.cloudera.org:8080/10102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Gerrit-Change-Number: 10102 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Apr 2018 00:59:26 + Gerrit-HasComments: No
[kudu-CR] Re-enable IWYU checks on HMS module
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10102 ) Change subject: Re-enable IWYU checks on HMS module .. Patch Set 3: Rebased. I'm a little confused, because I'm not getting iwyu errors before _or_ after applying this patch on an internal build machine. We'll see what gerrit says. I think the code cleanups in this patch are worth landing. -- To view, visit http://gerrit.cloudera.org:8080/10102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Gerrit-Change-Number: 10102 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Apr 2018 00:45:38 + Gerrit-HasComments: No
[kudu-CR] Re-enable IWYU checks on HMS module
Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10102 to look at the new patch set (#3). Change subject: Re-enable IWYU checks on HMS module .. Re-enable IWYU checks on HMS module IWYU previously had some issues with the HMS module[1], probably due to the Thrift generated classes. Now that IWYU has been upgraded[2] this is an attempt to re-enable the IWYU checks. This commit also includes some associated fixups. [1]: https://github.com/apache/kudu/commit/c09a6296181ecbb7d91687536f70fdbaac9fff5e [2]: https://github.com/apache/kudu/commit/bba339b11795d84da9c9d1473366ca561730c141 Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a --- M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_catalog.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/hms_client.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h M src/kudu/integration-tests/master_hms-itest.cc M src/kudu/master/catalog_manager.h 8 files changed, 43 insertions(+), 37 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/10102/3 -- To view, visit http://gerrit.cloudera.org:8080/10102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Gerrit-Change-Number: 10102 Gerrit-PatchSet: 3 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Re-enable IWYU checks on HMS module
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10102 ) Change subject: Re-enable IWYU checks on HMS module .. Patch Set 2: Is this still an issue after 72847ab5607b64d64bbe43edd942889e4e9f9fbf was committed? -- To view, visit http://gerrit.cloudera.org:8080/10102 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I63e5f1ca6a480f22ede83acde6baea8e2f563e8a Gerrit-Change-Number: 10102 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Apr 2018 00:24:53 + Gerrit-HasComments: No
[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/7362 ) Change subject: java: prohibit use of a KuduTable from an unassociated KuduClient .. Patch Set 3: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/7362/3/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java File java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java: http://gerrit.cloudera.org:8080/#/c/7362/3/java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java@76 PS3, Line 76: This operation is idempotent. Nit: with this change this statement becomes a little funky if debug is turned on. Also, with this change the code says 'don't call this method twice'. Maybe, remove this statement to avoid confusion? -- To view, visit http://gerrit.cloudera.org:8080/7362 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014 Gerrit-Change-Number: 7362 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Anonymous Coward #314 Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 25 Apr 2018 00:23:32 + Gerrit-HasComments: Yes
[kudu-CR] [WIP] [tools] ksck improvements [6/n]: Refactor printing
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10151 ) Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing .. Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc File src/kudu/tools/ksck_results.cc: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@88 PS5, Line 88: const string labels = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; static? It seems this function can be entered multiple times during kudu CLI lifecycle. Also, maybe change this to 'static const char* const' and use (sizeof(xxx)-1) instead of size(). http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@124 PS5, Line 124: string ServerHealthToString(KsckServerHealth sh) { nit: maybe, return 'const char* const' instead? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@177 PS5, Line 177: // Then, summarize the tablets by table nit: add a stop http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@268 PS5, Line 268: nit: two extra spaces http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@269 PS5, Line 269: "Name", "Status", "Total Tablets", : "Healthy", "Recovering", "Under-replicated", "Unavailable" consider adding replication factor, as it's done in https://gerrit.cloudera.org/#/c/10054/ http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.cc@273 PS5, Line 273: switch (ts.TableStatus()) { : case KsckCheckResult::HEALTHY: : status = "HEALTHY"; : break; : case KsckCheckResult::RECOVERING: : status = "RECOVERING"; : break; : case KsckCheckResult::UNDER_REPLICATED: : status = "UNDER-REPLICATED"; : break; : default: : status = "UNAVAILABLE"; : break; : } maybe, separate this into a function? -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 24 Apr 2018 23:52:34 + Gerrit-HasComments: Yes
[kudu-CR] java: prohibit use of a KuduTable from an unassociated KuduClient
Hello Alexey Serbin, David Ribeiro Alves, Jean-Daniel Cryans, Kudu Jenkins, Anonymous Coward #314, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7362 to look at the new patch set (#3). Change subject: java: prohibit use of a KuduTable from an unassociated KuduClient .. java: prohibit use of a KuduTable from an unassociated KuduClient This fixes a request-tracking issue with the following code anti-pattern in which a KuduTable associated with one client is used to create operations applied to another client's session: KuduClient client1 = KuduClientBuildernewClient(); KuduTable t = client1.openTable(...); KuduClient client2 = KuduClientBuildernewClient(); KuduSession s = client2.newSession(); s.apply(t.newUpdate()); This would cause sequence numbers to be generated out of the session's client's RequestTracker, but then marked complete in the operation's client's RequestTracker. This could cause any number of issues: - a cache "hit" on the server side might cause an operation to get an unrelated operation's response - a cache "miss" on the server side might result in an operation incorrectly being marked as already-responded and garbage collected, causing it to fail. This patch adds a Preconditions check for this issue and fixes some tests where it was triggered. Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014 --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduSession.java M java/kudu-client/src/main/java/org/apache/kudu/client/KuduRpc.java M java/kudu-client/src/main/java/org/apache/kudu/client/Operation.java M java/kudu-client/src/main/java/org/apache/kudu/client/RequestTracker.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITClientStress.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestRequestTracker.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestScannerMultiTablet.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java 9 files changed, 103 insertions(+), 57 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/7362/3 -- To view, visit http://gerrit.cloudera.org:8080/7362 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib4a977f36b9c7ba3758322a2216ce90208b5d014 Gerrit-Change-Number: 7362 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Anonymous Coward #314 Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [docs] Add a navigation link to the scaling guide
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10181 ) Change subject: [docs] Add a navigation link to the scaling guide .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/10181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3119d39b14c682fa40022ea5e5c199df15a033b2 Gerrit-Change-Number: 10181 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 24 Apr 2018 22:50:19 + Gerrit-HasComments: No
[kudu-CR] [WIP] [tools] ksck improvements [6/n]: Refactor printing
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10151 ) Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing .. Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10151/5//COMMIT_MSG@17 PS5, Line 17: - Removes the --consensus flag. ksck is far less useful without it and : it doesn't seem like having it brings any benefit. Any concerns about that in terms of backward compatibility? Also, maybe it's separating that change in a stand-alone changelist? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/integration-tests/cluster_verifier.cc File src/kudu/integration-tests/cluster_verifier.cc: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/integration-tests/cluster_verifier.cc@112 PS5, Line 112: RunAndPrintResults Would it make sense to separate that in two methods: 1. Run() 2. PrintResults() ? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc File src/kudu/tools/ksck-test.cc: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@845 PS5, Line 845: Status s = RunKsck(); : : // Every table we checked was healthy ;). : ASSERT_OK(s); Here an in other places: ASSERT_OK(RunKsck()) http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@858 PS5, Line 858: Status s = RunKsck(); : : ASSERT_OK(s); nit: why not just ASSERT_OK(RunKsck()) ? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck-test.cc@878 PS5, Line 878: Status s = RunKsck(); : : ASSERT_OK(s); ASSERT_OK(RunKsck()) ? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h File src/kudu/tools/ksck_results.h: http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@31 PS5, Line 31: why angle brackets instead of quotes? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@126 PS5, Line 126: // Returns an int signifying the "unhealthiness level" of 'sh'. : // Useful for sorting or comparing. It would be nice to mention what higher score value means, i.e. if server A has score 10 and server B has score 20, which server is 'healthier'? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@133 PS5, Line 133: address What if server has multiple RPC endpoints? http://gerrit.cloudera.org:8080/#/c/10151/5/src/kudu/tools/ksck_results.h@257 PS5, Line 257: const std::vector nit: why not constant reference? -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 24 Apr 2018 22:39:27 + Gerrit-HasComments: Yes
[kudu-CR] [docs] Add a navigation link to the scaling guide
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10181 Change subject: [docs] Add a navigation link to the scaling guide .. [docs] Add a navigation link to the scaling guide Change-Id: I3119d39b14c682fa40022ea5e5c199df15a033b2 --- M docs/support/jekyll-templates/document.html.erb 1 file changed, 1 insertion(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/10181/1 -- To view, visit http://gerrit.cloudera.org:8080/10181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3119d39b14c682fa40022ea5e5c199df15a033b2 Gerrit-Change-Number: 10181 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
[kudu-CR] KUDU-2191: Metadata Upgrade Tool
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 ) Change subject: KUDU-2191: Metadata Upgrade Tool .. Patch Set 6: (6 comments) http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG@9 PS6, Line 9: a nit: an http://gerrit.cloudera.org:8080/#/c/10075/6//COMMIT_MSG@13 PS6, Line 13: expect nit: expected http://gerrit.cloudera.org:8080/#/c/10075/6/build-support/iwyu.py File build-support/iwyu.py: http://gerrit.cloudera.org:8080/#/c/10075/6/build-support/iwyu.py@86 PS6, Line 86: "src/kudu/tools/tool_action_hms.cc", : "src/kudu/tools/kudu-tool-test.cc", nit: just in case if you haven't looked at that yet, Todd added a dependency of the IWYU-related targets on related Thrift auto-generated stuff, so this might be not necessary anymore. http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@92 PS6, Line 92: table_name What if 'table_name' is already a 'new' one? I.e., my concern is that this methods does not look re-enterable, but I might be missing something. As I understand, kudu_client->ListTables() would output not only legacy tables, right? http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@142 PS6, Line 142: Uris nit: URIs http://gerrit.cloudera.org:8080/#/c/10075/6/src/kudu/tools/tool_action_hms.cc@188 PS6, Line 188: .AddOptionalParameter("hive_metastore_uris") : .AddOptionalParameter("hive_metastore_sasl_enabled") : .AddOptionalParameter("hive_metastore_retry_count") : .AddOptionalParameter("hive_metastore_send_timeout") : .AddOptionalParameter("hive_metastore_recv_timeout") : .AddOptionalParameter("hive_metastore_conn_timeout") usability nit: does it make sense to remove the 'hive_metastore_' prefix ? If the sub-command is 'hms', doesn't it imply that all those options are about Hive metastore? If not, then maybe at least replace 'hive_metastore_' with 'hms_'? -- To view, visit http://gerrit.cloudera.org:8080/10075 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3 Gerrit-Change-Number: 10075 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Tue, 24 Apr 2018 20:57:44 + Gerrit-HasComments: Yes
[kudu-CR] [WIP] [tools] ksck improvements [6/n]: Refactor printing
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10151 ) Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing .. Patch Set 5: > Build Failed Unrelated flakes. -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 24 Apr 2018 20:43:10 + Gerrit-HasComments: No
[kudu-CR] iwyu: add proper dependency on thrift generated code, check for iwyu errors
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10161 ) Change subject: iwyu: add proper dependency on thrift generated code, check for iwyu errors .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/10161/1/build-support/iwyu.py File build-support/iwyu.py: http://gerrit.cloudera.org:8080/#/c/10161/1/build-support/iwyu.py@52 PS1, Line 52: r'^.+?:\d+:\d+:\s*' > the FATAL_ERROR case is already handled explicitly in _run_iwyu_tool. I add Ah, yes -- I found that yesterday as well after reviewing that patch. Thank you for addressing this tiny nit. -- To view, visit http://gerrit.cloudera.org:8080/10161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde Gerrit-Change-Number: 10161 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Apr 2018 19:59:41 + Gerrit-HasComments: Yes
[kudu-CR] iwyu: add proper dependency on thrift generated code, check for iwyu errors
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10161 ) Change subject: iwyu: add proper dependency on thrift generated code, check for iwyu errors .. iwyu: add proper dependency on thrift generated code, check for iwyu errors IWYU was giving incorrect results on the HMS modules when running on Jenkins. This was due to the 'iwyu' CMake target not depending on the thrift code generation. IWYU was continuing to run despite having 'missing include' errors. This patch adds the appropriate CMake dependency and also makes our IWYU wrapper check for such errors in the IWYU output. I also changed the IWYU wrapper to explicitly build the IWYU dependencies before running so that even if a user invokes it directly, they won't hit this issue. Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde Reviewed-on: http://gerrit.cloudera.org:8080/10161 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M CMakeLists.txt M build-support/iwyu.py 2 files changed, 19 insertions(+), 5 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/10161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde Gerrit-Change-Number: 10161 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [WIP] [tools] ksck improvements [6/n]: Refactor printing
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10151 to look at the new patch set (#5). Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing .. [WIP] [tools] ksck improvements [6/n]: Refactor printing This patch significantly refactors printing in ksck. It separates where checks are done from where results are printed. This separation will make it easier to provide machine-readable output and create tools based on the results of ksck checks, like auto-repair tools. There's also a couple of bonus changes: - Simplifies running all ksck checks and printing the results, as done by the ksck tool and ClusterVerifier. - Removes the --consensus flag. ksck is far less useful without it and it doesn't seem like having it brings any benefit. Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 --- M src/kudu/integration-tests/cluster_verifier.cc 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 A src/kudu/tools/ksck_results.cc A src/kudu/tools/ksck_results.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 966 insertions(+), 781 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/10151/5 -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 5 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley
[kudu-CR] WIP: bootstrap: don't re-acquire row locks for operations that are skipped
Todd Lipcon has abandoned this change. ( http://gerrit.cloudera.org:8080/3040 ) Change subject: WIP: bootstrap: don't re-acquire row locks for operations that are skipped .. Abandoned This still applies but I couldn't benchmark a very useful improvement here, so abandoning until we have better data -- To view, visit http://gerrit.cloudera.org:8080/3040 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I8d050965b8e882b91463af96714b558d7a6652de Gerrit-Change-Number: 3040 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] WIP: KUDU-1366: allow building against jemalloc
Todd Lipcon has abandoned this change. ( http://gerrit.cloudera.org:8080/6918 ) Change subject: WIP: KUDU-1366: allow building against jemalloc .. Abandoned With recent bug fixes in tcmalloc I think this isn't worth pursuing in the near or medium term -- To view, visit http://gerrit.cloudera.org:8080/6918 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I59f77e9f1ebdf6f1451e0f723a91cda4790c434b Gerrit-Change-Number: 6918 Gerrit-PatchSet: 10 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] tool: improve format for dumping a rowset
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/3946 ) Change subject: tool: improve format for dumping a rowset .. Patch Set 3: IWYU error due to a transitive include of the thrift stuff. I'll rebase this on the fix -- To view, visit http://gerrit.cloudera.org:8080/3946 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0f1d08e08d2a3d20a87e49bb5338bf0585bd8e40 Gerrit-Change-Number: 3946 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 24 Apr 2018 18:58:39 + Gerrit-HasComments: No
[kudu-CR] iwyu: add proper dependency on thrift generated code, check for iwyu errors
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10161 to look at the new patch set (#2). Change subject: iwyu: add proper dependency on thrift generated code, check for iwyu errors .. iwyu: add proper dependency on thrift generated code, check for iwyu errors IWYU was giving incorrect results on the HMS modules when running on Jenkins. This was due to the 'iwyu' CMake target not depending on the thrift code generation. IWYU was continuing to run despite having 'missing include' errors. This patch adds the appropriate CMake dependency and also makes our IWYU wrapper check for such errors in the IWYU output. I also changed the IWYU wrapper to explicitly build the IWYU dependencies before running so that even if a user invokes it directly, they won't hit this issue. Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde --- M CMakeLists.txt M build-support/iwyu.py 2 files changed, 19 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/10161/2 -- To view, visit http://gerrit.cloudera.org:8080/10161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde Gerrit-Change-Number: 10161 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] iwyu: add proper dependency on thrift generated code, check for iwyu errors
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10161 ) Change subject: iwyu: add proper dependency on thrift generated code, check for iwyu errors .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10161/1/build-support/iwyu.py File build-support/iwyu.py: http://gerrit.cloudera.org:8080/#/c/10161/1/build-support/iwyu.py@52 PS1, Line 52: r'^.+?:\d+:\d+: (fatal )?error:' > In $IWYU_SRC_ROOT/iwyu_test_util.py there is pattern for expected error/war the FATAL_ERROR case is already handled explicitly in _run_iwyu_tool. I added the \s* instead of ' ' -- To view, visit http://gerrit.cloudera.org:8080/10161 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie44f25ebc160c587eb11b4b4730095ee1e6c4fde Gerrit-Change-Number: 10161 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Apr 2018 18:03:28 + Gerrit-HasComments: Yes
[kudu-CR] [WIP] [tools] ksck improvements [6/n]: Refactor printing
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10151 ) Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing .. Patch Set 4: > Uploaded patch set 4. Junk rev with some logging to identify the cause of a test failure I can't reproduce. -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 24 Apr 2018 17:59:26 + Gerrit-HasComments: No
[kudu-CR] [WIP] [tools] ksck improvements [6/n]: Refactor printing
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10151 to look at the new patch set (#4). Change subject: [WIP] [tools] ksck improvements [6/n]: Refactor printing .. [WIP] [tools] ksck improvements [6/n]: Refactor printing This patch significantly refactors printing in ksck. It separates where checks are done from where results are printed. This separation will make it easier to provide machine-readable output and create tools based on the results of ksck checks, like auto-repair tools. There's also a couple of bonus changes: - Simplifies running all ksck checks and printing the results, as done by the ksck tool and ClusterVerifier. - Removes the --consensus flag. ksck is far less useful without it and it doesn't seem like having it brings any benefit. Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 --- M src/kudu/integration-tests/cluster_verifier.cc 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 A src/kudu/tools/ksck_results.cc A src/kudu/tools/ksck_results.h M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/tool_action_cluster.cc M src/kudu/tools/tool_action_tablet.cc 11 files changed, 968 insertions(+), 781 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/51/10151/4 -- To view, visit http://gerrit.cloudera.org:8080/10151 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id8de619996b6cd753e6a9c01b1b60810a873e609 Gerrit-Change-Number: 10151 Gerrit-PatchSet: 4 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] tool: improve format for dumping a rowset
Hello Will Berkeley, Tidy Bot, Kudu Jenkins, Adar Dembo, Dinesh Bhat, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3946 to look at the new patch set (#3). Change subject: tool: improve format for dumping a rowset .. tool: improve format for dumping a rowset This changes the output for 'kudu local_replica dump rowset' to be more human-readable. The output now looks like this: RowIdxInBlock: 0; Base: (int32 key=0, int32 int_val=0, string string_val="HelloWorld"); Undo Mutations: [@1(DELETE)]; Redo Mutations: []; RowIdxInBlock: 1; Base: (int32 key=1, int32 int_val=10, string string_val="HelloWorld"); Undo Mutations: [@2(DELETE)]; Redo Mutations: []; RowIdxInBlock: 2; Base: (int32 key=2, int32 int_val=20, string string_val="HelloWorld"); Undo Mutations: [@3(DELETE)]; Redo Mutations: []; ... rather than separately dumping each column block. Dumping individual blocks is still possible by using the cfile dump commands. Change-Id: I0f1d08e08d2a3d20a87e49bb5338bf0585bd8e40 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_local_replica.cc 2 files changed, 23 insertions(+), 217 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/46/3946/3 -- To view, visit http://gerrit.cloudera.org:8080/3946 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0f1d08e08d2a3d20a87e49bb5338bf0585bd8e40 Gerrit-Change-Number: 3946 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dinesh Bhat Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2214 Update logging to differentiate voting while copying/tombstoned
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10169 ) Change subject: KUDU-2214 Update logging to differentiate voting while copying/tombstoned .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10169/1/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/10169/1/src/kudu/consensus/raft_consensus.cc@1561 PS1, Line 1561: LOG_WITH_PREFIX_UNLOCKED(INFO) << "voting based on last-logged opid " > Sounds good. Thanks. I'll try this. Could you tell me why doing this btw? I Sure. The reason is that the log message today is misleading -- it says that it's voting while tombstoned but it's not tombstoned. This may lead the operator to thinking that a tablet is in a different state than it actually is. I would also be OK with just removing the words "while tombstoned" above and not changing anything else. Maybe Mike has an opinion about that. -- To view, visit http://gerrit.cloudera.org:8080/10169 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I07007601d0a86d6161065629ba167121a33635d6 Gerrit-Change-Number: 10169 Gerrit-PatchSet: 1 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 24 Apr 2018 17:09:14 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2214 Update logging to differentiate voting while copying/tombstoned
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10169 ) Change subject: KUDU-2214 Update logging to differentiate voting while copying/tombstoned .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10169/1/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/10169/1/src/kudu/consensus/raft_consensus.cc@1561 PS1, Line 1561: LOG_WITH_PREFIX_UNLOCKED(INFO) << "voting based on last-logged opid " > One idea here is to change the signature of this method so that instead of Sounds good. Thanks. I'll try this. Could you tell me why doing this btw? I feel like changing the signature is going affect quite a few use cases.? -- To view, visit http://gerrit.cloudera.org:8080/10169 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I07007601d0a86d6161065629ba167121a33635d6 Gerrit-Change-Number: 10169 Gerrit-PatchSet: 1 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 24 Apr 2018 16:49:36 + Gerrit-HasComments: Yes
[kudu-CR] iwyu: mute non-source files when using compilation DB
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10160 ) Change subject: iwyu: mute non-source files when using compilation DB .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10160/1/build-support/iwyu.py File build-support/iwyu.py: http://gerrit.cloudera.org:8080/#/c/10160/1/build-support/iwyu.py@149 PS1, Line 149: not rel.startswith('src/') > Is that for various auto-generated stuff under build directory? yea -- To view, visit http://gerrit.cloudera.org:8080/10160 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibd00ab084536353d2a9a733fc0a11197b8757df4 Gerrit-Change-Number: 10160 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Apr 2018 16:44:19 + Gerrit-HasComments: Yes
[kudu-CR] iwyu: mute non-source files when using compilation DB
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10160 ) Change subject: iwyu: mute non-source files when using compilation DB .. iwyu: mute non-source files when using compilation DB When running 'iwyu.py --all', it would report various IWYU violations in included header files. This mutes any reports of issues outside of our src directory. Change-Id: Ibd00ab084536353d2a9a733fc0a11197b8757df4 Reviewed-on: http://gerrit.cloudera.org:8080/10160 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M build-support/iwyu.py 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/10160 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ibd00ab084536353d2a9a733fc0a11197b8757df4 Gerrit-Change-Number: 10160 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] iwyu: pass source root into fix includes
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10159 ) Change subject: iwyu: pass source root into fix_includes .. iwyu: pass source root into fix_includes fix_includes tries to identify the "main compilation unit header" for each file using some heuristics. For example, "foo-test.cc" is associated with "foo.h". This was previously broken due to the fact that the tool runs from the build/ directory instead of the source directory, so it saw the CC file as "../../src/kudu/.../foo.cc" while it saw the header file as "kudu/.../foo.h". This adds a --source_root parameter to fix_includes so that the source files can be relativized to this path prior to fixing. Change-Id: Iac9f0b47b9a3c8180e178ed59863d33840434433 Reviewed-on: http://gerrit.cloudera.org:8080/10159 Tested-by: Todd Lipcon Reviewed-by: Alexey Serbin --- M build-support/iwyu.py M build-support/iwyu/fix_includes.py 2 files changed, 24 insertions(+), 14 deletions(-) Approvals: Todd Lipcon: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/10159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iac9f0b47b9a3c8180e178ed59863d33840434433 Gerrit-Change-Number: 10159 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] iwyu: workaround missing IWYU results when using Ninja
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9554 ) Change subject: iwyu: workaround missing IWYU results when using Ninja .. iwyu: workaround missing IWYU results when using Ninja This adds a workaround so that IWYU reports the same results whether cmake is invoked with the Makefile generator or the Ninja generator. Tested by verifying that IWYU on my laptop now reports the same issues that Jenkins was reporting precommit on a recent in-flight patch. Change-Id: I092cab794fd20a39fcfbbbe52f4c325fac2d149c Reviewed-on: http://gerrit.cloudera.org:8080/9554 Tested-by: Todd Lipcon Reviewed-by: Alexey Serbin --- M build-support/iwyu/iwyu_tool.py 1 file changed, 43 insertions(+), 0 deletions(-) Approvals: Todd Lipcon: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/9554 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I092cab794fd20a39fcfbbbe52f4c325fac2d149c Gerrit-Change-Number: 9554 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] iwyu: pass source root into fix includes
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10159 ) Change subject: iwyu: pass source root into fix_includes .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10159/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10159/1//COMMIT_MSG@15 PS1, Line 15: . > nit: an extra dot? no, I mean '...' as in 'fill in some path here' -- To view, visit http://gerrit.cloudera.org:8080/10159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iac9f0b47b9a3c8180e178ed59863d33840434433 Gerrit-Change-Number: 10159 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 24 Apr 2018 16:43:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2214 Update logging to differentiate voting while copying/tombstoned
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10169 ) Change subject: KUDU-2214 Update logging to differentiate voting while copying/tombstoned .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/10169/1/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: http://gerrit.cloudera.org:8080/#/c/10169/1/src/kudu/consensus/raft_consensus.cc@1561 PS1, Line 1561: LOG_WITH_PREFIX_UNLOCKED(INFO) << "voting based on last-logged opid " One idea here is to change the signature of this method so that instead of just taking in OpId last_logged_opid, it instead takes some simple struct like: struct TabletVotingState { optional last_logged_op_id; TabletDataState data_state; }; and then you can modify this message to switch on the passed-in data state. -- To view, visit http://gerrit.cloudera.org:8080/10169 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I07007601d0a86d6161065629ba167121a33635d6 Gerrit-Change-Number: 10169 Gerrit-PatchSet: 1 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 24 Apr 2018 16:21:12 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2214 Update logging to differentiate voting while copying/tombstoned
Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/10169 ) Change subject: KUDU-2214 Update logging to differentiate voting while copying/tombstoned .. Patch Set 1: (1 comment) So far I don't know the best approach to solve this issue. Hoping for suggestions. Thanks. http://gerrit.cloudera.org:8080/#/c/10169/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/10169/1/src/kudu/tserver/tablet_service.cc@1006 PS1, Line 1006: LOG(INFO) << "Attempting to vote while " > does this end up doubling the number of logs during a vote? yes actually. -- To view, visit http://gerrit.cloudera.org:8080/10169 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I07007601d0a86d6161065629ba167121a33635d6 Gerrit-Change-Number: 10169 Gerrit-PatchSet: 1 Gerrit-Owner: Fengling Wang Gerrit-Reviewer: Fengling Wang Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Tue, 24 Apr 2018 15:44:47 + Gerrit-HasComments: Yes
[kudu-CR] Use Gradle by default in build-and-test.sh
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/10162 ) Change subject: Use Gradle by default in build-and-test.sh .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/10162/3/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/10162/3/build-support/jenkins/build-and-test.sh@392 PS3, Line 392: GRADLE_FLAGS="$GRADLE_FLAGS --console=plain --no-daemon --continue" > What's the significance of --continue? Why is it needed here? I will add this in a separate patch, but I am just experimenting here. continue makes sure the whole build runs regardless of failure so all failures are reported. -- To view, visit http://gerrit.cloudera.org:8080/10162 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia919af374f4909d2b8eb38d6c7040704eb24b457 Gerrit-Change-Number: 10162 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Apr 2018 15:27:43 + Gerrit-HasComments: Yes
[kudu-CR] Use Gradle by default in build-and-test.sh
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10162 ) Change subject: Use Gradle by default in build-and-test.sh .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/10162/3/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/10162/3/build-support/jenkins/build-and-test.sh@392 PS3, Line 392: GRADLE_FLAGS="$GRADLE_FLAGS --console=plain --no-daemon --continue" What's the significance of --continue? Why is it needed here? -- To view, visit http://gerrit.cloudera.org:8080/10162 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia919af374f4909d2b8eb38d6c7040704eb24b457 Gerrit-Change-Number: 10162 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Apr 2018 15:26:17 + Gerrit-HasComments: Yes
[kudu-CR] Use Gradle by default in build-and-test.sh
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/10162 to look at the new patch set (#3). Change subject: Use Gradle by default in build-and-test.sh .. Use Gradle by default in build-and-test.sh Change-Id: Ia919af374f4909d2b8eb38d6c7040704eb24b457 --- M build-support/jenkins/build-and-test.sh 1 file changed, 5 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/10162/3 -- To view, visit http://gerrit.cloudera.org:8080/10162 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia919af374f4909d2b8eb38d6c7040704eb24b457 Gerrit-Change-Number: 10162 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins