[kudu-CR] Bump test timeout for flex partitioning-itest
Todd Lipcon has submitted this change and it was merged. Change subject: Bump test timeout for flex_partitioning-itest .. Bump test timeout for flex_partitioning-itest In precommit runs, this test is sharded 8 ways so it never has issues with timeouts. However, in some Jenkins builds that don't use dist-test, it can time out, especially when TSAN is enabled. This adds functionality to our ADD_KUDU_TEST() cmake function to specify a timeout and plumbs that timeout through to our test wrapper shell script. Tested by verifying that the appropriate environment variable gets set in the environment of the running test. Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Reviewed-on: http://gerrit.cloudera.org:8080/7814 Reviewed-by: Adar Dembo Tested-by: Todd Lipcon --- M CMakeLists.txt M build-support/run-test.sh M src/kudu/integration-tests/CMakeLists.txt 3 files changed, 46 insertions(+), 4 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Todd Lipcon: Verified -- To view, visit http://gerrit.cloudera.org:8080/7814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Bump test timeout for flex partitioning-itest
Todd Lipcon has posted comments on this change. Change subject: Bump test timeout for flex_partitioning-itest .. Patch Set 3: Verified+1 Release build hit KUDU-2089 (java test retry left tmp files, causing the build to fail) -- To view, visit http://gerrit.cloudera.org:8080/7814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java] Reuse JVM across tests
Todd Lipcon has posted comments on this change. Change subject: [java] Reuse JVM across tests .. Patch Set 1: > Having the Java tests run in dist-test would be really nice. We can't > parallelize the unit tests locally because they have collisions on the > MiniCluster usage. yea, that could be solved the same way that the C++ client solves it, though (having the minicluster daemons bind to 127.a.b.c rather than 127.0.0.1). Doesn't work on OSX but c'est la vie -- To view, visit http://gerrit.cloudera.org:8080/7825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java] Reuse JVM across tests
Grant Henke has posted comments on this change. Change subject: [java] Reuse JVM across tests .. Patch Set 1: Having the Java tests run in dist-test would be really nice. We can't parallelize the unit tests locally because they have collisions on the MiniCluster usage. -- To view, visit http://gerrit.cloudera.org:8080/7825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java] Reuse JVM across tests
Grant Henke has posted comments on this change. Change subject: [java] Reuse JVM across tests .. Patch Set 1: Yeah, I agree with "guaranteed isolation" being more reliable. This patch came as a result of the gradle patch where weird issues were seen because I didn't use a different JVM per test. We can leave 1 JVM per test class, but these teardown fixes are still valid. -- To view, visit http://gerrit.cloudera.org:8080/7825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java] Reuse JVM across tests
Grant Henke has posted comments on this change. Change subject: [java] Reuse JVM across tests .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/7825/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java: Line 70: public void testLowTimeoutScans() throws Exception { I broke these tests out because it looks like sometimes a warm client could actually succeed in 1ms. Its also nice to have a method for each thing tested. http://gerrit.cloudera.org:8080/#/c/7825/1/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala: Line 350: def clear(): Unit = { Having cached connection that outlived the client was causing issues when we started a whole new cluster with a new certificates in later tests. Is their any real-world issue having JVM wide cached connections like this could cause? -- To view, visit http://gerrit.cloudera.org:8080/7825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [java] Reuse JVM across tests
Todd Lipcon has posted comments on this change. Change subject: [java] Reuse JVM across tests .. Patch Set 1: Not sure how I feel about this change. In my experience these kinds of things usually cause more hard-to-diagnose bugs than they are worth -- eg random tests end up leaving threads running in failure scenarios, and those threads end up logging stuff captured by future tests, interfering with them, etc. If we want to improve test runtime I think we're better off focusing on distributed execution or parallel execution across many VMs, but still having a one-vm-per-test. -- To view, visit http://gerrit.cloudera.org:8080/7825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java] Reuse JVM across tests
Adar Dembo has posted comments on this change. Change subject: [java] Reuse JVM across tests .. Patch Set 1: (1 comment) Just passing through with a question. http://gerrit.cloudera.org:8080/#/c/7825/1//COMMIT_MSG Commit Message: Line 7: [java] Reuse JVM across tests Could you measure the change in test running time and note it here? -- To view, visit http://gerrit.cloudera.org:8080/7825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] catalog manager: don't log deleted tables/tablets at startup
Hello Dan Burkert, Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7826 to review the following change. Change subject: catalog_manager: don't log deleted tables/tablets at startup .. catalog_manager: don't log deleted tables/tablets at startup On long-lived clusters this list can get quite long. While I was in the area, I made some other cosmetic improvements: - Applying strings::Substitute() to perforated LOG statements. - Replacing 'OVERRIDE' with 'override'. - Removing 'virtual' from overriden methods. - Removing unnecessary std:: prefixes. - A few other miscellaneous tweaks. Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306 --- M src/kudu/master/catalog_manager.cc M src/kudu/master/sys_catalog.h 2 files changed, 191 insertions(+), 205 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/26/7826/1 -- To view, visit http://gerrit.cloudera.org:8080/7826 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert
[kudu-CR] [java] Reuse JVM across tests
Grant Henke has uploaded a new change for review. http://gerrit.cloudera.org:8080/7825 Change subject: [java] Reuse JVM across tests .. [java] Reuse JVM across tests Currently each java test is run in a separate JVM.This is extra overhead and can cause the tests torun longer. This patch ensures “shared” resources arecleaned up properly across tests and reconfigures thetest runs to reuse the JVM. Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c --- M java/gradle/tests.gradle M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestTimeouts.java M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduContextTest.scala M java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/TestContext.scala M java/pom.xml 7 files changed, 50 insertions(+), 21 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/7825/1 -- To view, visit http://gerrit.cloudera.org:8080/7825 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I8e4c6d53054d5da4f2ecf9f4130a51df13d4fa6c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke
[kudu-CR] java-client: improve error messages when failing to connect to secure cluster
Hello Jean-Daniel Cryans, Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7824 to review the following change. Change subject: java-client: improve error messages when failing to connect to secure cluster .. java-client: improve error messages when failing to connect to secure cluster Cleans up the error handling in Connection.java so that is passes through non-recoverable exceptions, and improves the error which results from attempting to connect to a secure cluster with an unauthenticate client. Example of the new error: org.apache.kudu.client.NonRecoverableException: Couldn't find a valid master in (nightly512-1.gce.cloudera.com:7051). Exceptions received: org.apache.kudu.client.NonRecoverableException: Server requires Kerberos, but this client is not authenticated (kinit) Change-Id: I41c3229dcda284dce57cb6f6930efe3b50fa9698 --- M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java M java/kudu-client/src/main/java/org/apache/kudu/client/Negotiator.java 2 files changed, 66 insertions(+), 51 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/24/7824/1 -- To view, visit http://gerrit.cloudera.org:8080/7824 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I41c3229dcda284dce57cb6f6930efe3b50fa9698 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Jean-Daniel Cryans
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Adar Dembo has submitted this change and it was merged. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate new blocks in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API, Finalize(), to Block Manager, so that an LBM container can be reused without flushing in-flight writable blocks to disk. This can bring down the log block container number and disk space consumption for first few flush/compaction operations when using log block manager. I performed manual test which creates a table with two tablets, 'kudu perf loadgen localhost --num-rows-per-thread=2000 --num-threads=10 --keep-auto-table --table_num_buckets=2'. With this patch, 'log_block_manager_containers' dropped from 56 to 7. A design doc can be found here: https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Reviewed-on: http://gerrit.cloudera.org:8080/7207 Reviewed-by: Adar Dembo Tested-by: Adar Dembo --- M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.cc M src/kudu/fs/block_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/multi_column_writer.cc M src/kudu/tablet/multi_column_writer.h 20 files changed, 615 insertions(+), 453 deletions(-) Approvals: Adar Dembo: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 35 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Adar Dembo has submitted this change and it was merged. Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. KUDU-1726: Avoid fsync-per-block in tablet copy This patch incorporates BlockTransaction API into tablet copy, to avoid fsync() on each block during copying. Blocks are synced to disk together once the tablet copy is complete. It also introduces a new block manager metric 'total_disk_sync' to keep track of block data disk synchronization count. I did a manual test to copy tablet with size of ~37GB from one tablet server to another on el6 with ext4. Each tablet server has its own dedicated single disk. 'kudu remote_replica copy c53ffbc2ede84b6d9af2da607024d131 localhost:3334 localhost:3335' With this change, the operation time dropped down from ~718s to ~523s. Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Reviewed-on: http://gerrit.cloudera.org:8080/7701 Reviewed-by: Adar Dembo Tested-by: Adar Dembo --- M src/kudu/fs/block_manager_metrics.cc M src/kudu/fs/block_manager_metrics.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/log_block_manager.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h 7 files changed, 71 insertions(+), 17 deletions(-) Approvals: Adar Dembo: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Adar Dembo has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 34: Verified+1 We can't let IWYU get in the way of progress. -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 34 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Adar Dembo has posted comments on this change. Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. Patch Set 7: Verified+1 Just say no to IWYU. -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] Fix flaky ts recovery-itest TestChangeMaxCellSize
Andrew Wong has posted comments on this change. Change subject: Fix flaky ts_recovery-itest TestChangeMaxCellSize .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7820/1/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 1014: if (!replica->error().ok()) { > This has an ABA problem, potentially. How about: Hrm, sure. Setting error _should_ be permanent (in that it shouldn't go back to OK), but it's a reasonable safeguard against further state-transience changes. -- To view, visit http://gerrit.cloudera.org:8080/7820 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Fix flaky ts recovery-itest TestChangeMaxCellSize
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7820 to look at the new patch set (#2). Change subject: Fix flaky ts_recovery-itest TestChangeMaxCellSize .. Fix flaky ts_recovery-itest TestChangeMaxCellSize In TSTabletManager::CreateReportedTabletPB(), each replica's state is reported, and if this state is FAILED, it additionally reports the error state that caused it to fail. However, replica state is transient, and can, on rare occasion, change between these two retrievals of replica state. This led to a DCHECK failure in ts_recovery-itest when parsing a report, as reports with errors are expected to be FAILED. The failure was consistent with the following sequence of events: 1. The tablet is opened, hits an error, and starts shutting down 2. The report notes the replica state (STOPPING) 3. The tablet finishes shutting down and switches its state (FAILED) 4. The report, seeing the replica is FAILED, notes the error that caused the shut down 5. The catalog manager goes through the report, and notes the error tacked onto the report and the fact that the tablet is STOPPING instead of FAILED. This fails the DCHECK, as logged below: BlockManagerType/TsRecoveryITest.TestChangeMaxCellSize/0: catalog_manager.cc:2489] Check failed: report.state() == tablet::FAILED (3 vs. 2) This sequence may be more frequent since a9d17c0, which enforces the tablet stops (i.e. go into the STOPPING state and shut down the replica's internals) before going into the FAILED state, instead of going directly to the FAILED state. Validated by injecting a pause in the CreateReportedTabletPB between the two calls and hitting the DCHECK failure. This is fixed by reporting the error regardless of state and removing this DCHECK. Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6 --- M src/kudu/master/catalog_manager.cc M src/kudu/tserver/ts_tablet_manager.cc 2 files changed, 4 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/7820/2 -- To view, visit http://gerrit.cloudera.org:8080/7820 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Adar Dembo has posted comments on this change. Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] Fix flaky ts recovery-itest TestChangeMaxCellSize
Mike Percy has posted comments on this change. Change subject: Fix flaky ts_recovery-itest TestChangeMaxCellSize .. Patch Set 1: (1 comment) Yeah, that DCHECK is not useful. http://gerrit.cloudera.org:8080/#/c/7820/1/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: Line 1014: if (!replica->error().ok()) { This has an ABA problem, potentially. How about: const Status& error = replica->error(); if (!error.ok()) { StatusToPB(reported_tablet->mutable_error(), error_status); } -- To view, visit http://gerrit.cloudera.org:8080/7820 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [tests] fix flakiness in catalog manager tsk-itest
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/7823 Change subject: [tests] fix flakiness in catalog_manager_tsk-itest .. [tests] fix flakiness in catalog_manager_tsk-itest After recent updates the catalog_manager_tsk-itest became unstable in TSAN configuration. The failure scenario looks like the following: * 3 masters are running with --raft_enable_pre_election=false flag and short raft heartbeat interval. * The test client sends CreateTable request. The server side completes the request, but once the storm of master re-elections starts, the client cannot get the authoritative answer on its IsCreateTableDone requests because master leadership changes every 40-100ms. As as result, after retrying IsCreateTableDone request many times, the client times out on its CreateTable request. Making the raft heartbeat interval higher helps keeping the master leadership intervals longer, so the client has better chance of getting authoritative response from the masters. Change-Id: I3731fb560923d82d2fb64ac7b829080f99a0681e --- M src/kudu/integration-tests/catalog_manager_tsk-itest.cc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/7823/1 -- To view, visit http://gerrit.cloudera.org:8080/7823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I3731fb560923d82d2fb64ac7b829080f99a0681e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Hao Hao has posted comments on this change. Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/7701/6//COMMIT_MSG Commit Message: Line 17: has its own dedicated single disk. > And just to be clear, each tablet server was using its own, dedicated disk, Right, added the commit message accordingly. -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Hello Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7701 to look at the new patch set (#7). Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. KUDU-1726: Avoid fsync-per-block in tablet copy This patch incorporates BlockTransaction API into tablet copy, to avoid fsync() on each block during copying. Blocks are synced to disk together once the tablet copy is complete. It also introduces a new block manager metric 'total_disk_sync' to keep track of block data disk synchronization count. I did a manual test to copy tablet with size of ~37GB from one tablet server to another on el6 with ext4. Each tablet server has its own dedicated single disk. 'kudu remote_replica copy c53ffbc2ede84b6d9af2da607024d131 localhost:3334 localhost:3335' With this change, the operation time dropped down from ~718s to ~523s. Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 --- M src/kudu/fs/block_manager_metrics.cc M src/kudu/fs/block_manager_metrics.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/log_block_manager.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h 7 files changed, 71 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/7701/7 -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 34: Not sure why IWYU is complaining about including ext/alloc_traits.h for log_block_manager.cc, etc? -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 34 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Adar Dembo has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 34: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 34 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#34). Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate new blocks in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API, Finalize(), to Block Manager, so that an LBM container can be reused without flushing in-flight writable blocks to disk. This can bring down the log block container number and disk space consumption for first few flush/compaction operations when using log block manager. I performed manual test which creates a table with two tablets, 'kudu perf loadgen localhost --num-rows-per-thread=2000 --num-threads=10 --keep-auto-table --table_num_buckets=2'. With this patch, 'log_block_manager_containers' dropped from 56 to 7. A design doc can be found here: https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.cc M src/kudu/fs/block_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/multi_column_writer.cc M src/kudu/tablet/multi_column_writer.h 20 files changed, 615 insertions(+), 453 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/34 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 34 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] gradle: target JRE 7 compatibility
Dan Burkert has submitted this change and it was merged. Change subject: gradle: target JRE 7 compatibility .. gradle: target JRE 7 compatibility Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29 Reviewed-on: http://gerrit.cloudera.org:8080/7785 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon Reviewed-by: Grant Henke Reviewed-by: Adar Dembo --- M java/gradle.properties M java/gradle/compile.gradle 2 files changed, 7 insertions(+), 5 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Grant Henke: Looks good to me, but someone else must approve Todd Lipcon: Looks good to me, but someone else must approve Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7785 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Adar Dembo has posted comments on this change. Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] gradle: target JRE 7 compatibility
Adar Dembo has posted comments on this change. Change subject: gradle: target JRE 7 compatibility .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7785 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Dan Burkert has posted comments on this change. Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7821/1/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: Line 131: // Hard code the SSL_OP_NO_TLSv1 and SSL_OP_NO_TLSv1_1 flags from OpenSSL > Makes sense, but rather than the macros here, can we do something like this Done -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Dan Burkert has uploaded a new patch set (#2). Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. rpc: allow setting --rpc_tls_min_protocol on older RHEL versions Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 --- M src/kudu/security/tls_context.cc 1 file changed, 25 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/7821/2 -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Adar Dembo has posted comments on this change. Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7821/1/src/kudu/security/tls_context.cc File src/kudu/security/tls_context.cc: Line 131: // Hard code the SSL_OP_NO_TLSv1 and SSL_OP_NO_TLSv1_1 flags from OpenSSL Makes sense, but rather than the macros here, can we do something like this earlier, after including the openssl headers: #ifndef SSL_OP_NO_TLSv1 #define SSL_OP_NO_TLSv1 0x0400U #endif #ifndef SSL_OP_NO_TLSv1_1 #define SSL_OP_NO_TLSv1_1 0x1000U #endif That should improve readability in the code down here. -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] rpc: allow setting --rpc tls min protocol on older RHEL versions
Hello Henry Robinson, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7821 to review the following change. Change subject: rpc: allow setting --rpc_tls_min_protocol on older RHEL versions .. rpc: allow setting --rpc_tls_min_protocol on older RHEL versions Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 --- M src/kudu/security/tls_context.cc 1 file changed, 20 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/7821/1 -- To view, visit http://gerrit.cloudera.org:8080/7821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ic61f31788d63072fae609c6a2186e52d5e2467b7 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Henry Robinson Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add more trace counters and timings
Adar Dembo has posted comments on this change. Change subject: Add more trace counters and timings .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7819/1/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: Line 764: virtual Status Read(uint64_t offset, Slice* result) const OVERRIDE { > I think this already gets traced at the LBM level, no? Yeah but you don't like having traces at multiple levels? What if we have heavy non-LBM RWFile usage in the future? -- To view, visit http://gerrit.cloudera.org:8080/7819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie99bc9e06be682e1595745f154b7dded3903bd6e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Add more trace counters and timings
Todd Lipcon has posted comments on this change. Change subject: Add more trace counters and timings .. Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/7819/1/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: Line 764: virtual Status Read(uint64_t offset, Slice* result) const OVERRIDE { > No tracing for reads/writes? I think this already gets traced at the LBM level, no? Line 786: TRACE_EVENT1("io", "PosixRWFile::PreAllocate", "path", filename_); > Want to include the offset/length? this is just moved from below, but sure. -- To view, visit http://gerrit.cloudera.org:8080/7819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie99bc9e06be682e1595745f154b7dded3903bd6e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Bump test timeout for flex partitioning-itest
Adar Dembo has posted comments on this change. Change subject: Bump test timeout for flex_partitioning-itest .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Add more trace counters and timings
Adar Dembo has posted comments on this change. Change subject: Add more trace counters and timings .. Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/7819/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS1, Line 1245: int64_t dur = end_time - start_time; : TRACE_COUNTER_INCREMENT("lbm_write_time_us", dur); : const char* counter = BUCKETED_COUNTER_NAME("lbm_writes", dur); : TRACE_COUNTER_INCREMENT(counter, 1); Maybe move this into the container too, for symmetry? PS1, Line 1458: int64_t dur = end_time - start_time; : TRACE_COUNTER_INCREMENT("lbm_read_time_us", dur); : : const char* counter = BUCKETED_COUNTER_NAME("lbm_reads", dur); : TRACE_COUNTER_INCREMENT(counter, 1); And this. http://gerrit.cloudera.org:8080/#/c/7819/1/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: Line 764: virtual Status Read(uint64_t offset, Slice* result) const OVERRIDE { No tracing for reads/writes? Line 786: TRACE_EVENT1("io", "PosixRWFile::PreAllocate", "path", filename_); Want to include the offset/length? Line 827: TRACE_EVENT1("io", "PosixRWFile::PunchHole", "path", filename_); Could include offset/length here too. Line 863: TRACE_EVENT1("io", "PosixRWFile::Flush", "path", filename_); And here too. Line 902: TRACE_EVENT1("io", "PosixRWFile::Close", "path", filename_); Trace close_us and number of close operations? Line 1280: TRACE_EVENT1("io", "PosixEnv::GetFileSizeOnDisk", "path", fname); Trace micros and number of operations? Same for the other stat-based functions below. -- To view, visit http://gerrit.cloudera.org:8080/7819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie99bc9e06be682e1595745f154b7dded3903bd6e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] Fix flaky ts recovery-itest TestChangeMaxCellSize
Andrew Wong has uploaded a new change for review. http://gerrit.cloudera.org:8080/7820 Change subject: Fix flaky ts_recovery-itest TestChangeMaxCellSize .. Fix flaky ts_recovery-itest TestChangeMaxCellSize In TSTabletManager::CreateReportedTabletPB(), each replica's state is reported, and if this state is FAILED, it additionally reports the error state that caused it to fail. However, replica state is transient, and can, on rare occasion, change between these two retrievals of replica state. This led to a DCHECK failure in ts_recovery-itest when parsing a report, as reports with errors are expected to be FAILED. The failure was consistent with the following sequence of events: 1. The tablet is opened, hits an error, and starts shutting down 2. The report notes the replica state (STOPPING) 3. The tablet finishes shutting down and switches its state (FAILED) 4. The report, seeing the replica is FAILED, notes the error that caused the shut down 5. The catalog manager goes through the report, and notes the error tacked onto the report and the fact that the tablet is STOPPING instead of FAILED. This fails the DCHECK, as logged below: BlockManagerType/TsRecoveryITest.TestChangeMaxCellSize/0: catalog_manager.cc:2489] Check failed: report.state() == tablet::FAILED (3 vs. 2) This sequence may be more frequent since a9d17c0, which enforces the tablet stop (i.e. go into the STOPPING state and shut down the replica's internals) before going into the FAILED state, instead of going directly to the FAILED state. This was validated by injecting a pause in the CreateReportedTabletPB between the two calls and hitting the DCHECK failure. This is fixed by reporting the error regardless of state and removing this DCHECK. Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6 --- M src/kudu/master/catalog_manager.cc M src/kudu/tserver/ts_tablet_manager.cc 2 files changed, 2 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/7820/1 -- To view, visit http://gerrit.cloudera.org:8080/7820 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I6f0a6b19756777e5f4081ef6c8cb5af4ecc8a3d6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong
[kudu-CR] Bump test timeout for flex partitioning-itest
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7814 to look at the new patch set (#3). Change subject: Bump test timeout for flex_partitioning-itest .. Bump test timeout for flex_partitioning-itest In precommit runs, this test is sharded 8 ways so it never has issues with timeouts. However, in some Jenkins builds that don't use dist-test, it can time out, especially when TSAN is enabled. This adds functionality to our ADD_KUDU_TEST() cmake function to specify a timeout and plumbs that timeout through to our test wrapper shell script. Tested by verifying that the appropriate environment variable gets set in the environment of the running test. Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 --- M CMakeLists.txt M build-support/run-test.sh M src/kudu/integration-tests/CMakeLists.txt 3 files changed, 46 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/7814/3 -- To view, visit http://gerrit.cloudera.org:8080/7814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Bump test timeout for flex partitioning-itest
Todd Lipcon has posted comments on this change. Change subject: Bump test timeout for flex_partitioning-itest .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7814/2/CMakeLists.txt File CMakeLists.txt: Line 673: set(ARG_TIMEOUT 900) > Maybe a comment about how this should be kept in-sync with the default valu Done -- To view, visit http://gerrit.cloudera.org:8080/7814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Add more trace counters and timings
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7819 to review the following change. Change subject: Add more trace counters and timings .. Add more trace counters and timings This afternoon I was looking at some DeleteTablet calls that were taking longer than expected. The trace metrics were rather incomplete, so it wasn't clear where the time was coming from. This adds some more tracing to various code paths related to IO and tablet metadata. Change-Id: Ie99bc9e06be682e1595745f154b7dded3903bd6e --- M src/kudu/fs/log_block_manager.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/util/env_posix.cc 3 files changed, 65 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/7819/1 -- To view, visit http://gerrit.cloudera.org:8080/7819 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ie99bc9e06be682e1595745f154b7dded3903bd6e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo
[kudu-CR] Bump test timeout for flex partitioning-itest
Adar Dembo has posted comments on this change. Change subject: Bump test timeout for flex_partitioning-itest .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7814/2/CMakeLists.txt File CMakeLists.txt: Line 673: set(ARG_TIMEOUT 900) Maybe a comment about how this should be kept in-sync with the default value in run-test.sh, and a comment there too? -- To view, visit http://gerrit.cloudera.org:8080/7814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Adar Dembo has posted comments on this change. Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/7701/6//COMMIT_MSG Commit Message: Line 17: has a single disk. And just to be clear, each tablet server was using its own, dedicated disk, right? The two weren't sharing the same disk? -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] [security] avoid kerberos ticket renewal and only reacquire
Sailesh Mukil has posted comments on this change. Change subject: [security] avoid kerberos ticket renewal and only reacquire .. Patch Set 1: > I think there is mention of 'renew' in init.h. Also I think we can > remove the renew_lifetime handling in mini_kdc.{h,cc} as well as > security-faults-itest.cc. I removed the mention of renew in init.* and other relevant places. However, I think it will be beneficial to leave in the renew_lifetime in the mini_kdc just to ensure that it works under different configurations, if and when we choose to add more tests that use the minikdc in the future. For eg: Add more tests to configure kerberos differently to attempt to catch issues like KUDU-2032. What do you think? -- To view, visit http://gerrit.cloudera.org:8080/7810 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e5225de332ba785e3a73014b8418cfd4059fe07 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Bump test timeout for flex partitioning-itest
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7814 to look at the new patch set (#2). Change subject: Bump test timeout for flex_partitioning-itest .. Bump test timeout for flex_partitioning-itest In precommit runs, this test is sharded 8 ways so it never has issues with timeouts. However, in some Jenkins builds that don't use dist-test, it can time out, especially when TSAN is enabled. This adds functionality to our ADD_KUDU_TEST() cmake function to specify a timeout and plumbs that timeout through to our test wrapper shell script. Tested by verifying that the appropriate environment variable gets set in the environment of the running test. Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 --- M CMakeLists.txt M src/kudu/integration-tests/CMakeLists.txt 2 files changed, 40 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/7814/2 -- To view, visit http://gerrit.cloudera.org:8080/7814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Bump test timeout for flex partitioning-itest
Todd Lipcon has posted comments on this change. Change subject: Bump test timeout for flex_partitioning-itest .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/7814/1/CMakeLists.txt File CMakeLists.txt: Line 673: set(ARG_TIMEOUT 900) > Can we avoid defining this default value both here and in run-test.sh? Give Yea, I considered doing this (not passing any TIMEOUT if there is nothing set). However I thought it was nice to have the "belt and suspenders" timeout structure here -- if for some reason the KuduTest timeout mechanism gets stuck, it's nice to have ctest applying a timeout as well. This isn't totally academic - we've seen cases where the 'pstack' which comes when a test timeout hits some bug in gdb which makes it hang, and then the entire build fails. So having ctest kill the test 30 seconds later would prevent that. Line 707: # Set the ctest timeout to be a bit longer than the timeout we pass to > Why bother with the ctest timeout at all, given that the run-test.sh timeou yea, see above Line 714: if("${CUR_TEST_ENV}" STREQUAL "NOTFOUND") > does if(NOT CUR_TEST_ENV) work? ah, I think it might, will try that. -- To view, visit http://gerrit.cloudera.org:8080/7814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Hao Hao has posted comments on this change. Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/7701/1//COMMIT_MSG Commit Message: PS1, Line 13: to keep track of block data disk synchronization count. : : I did a m > What about the disk setup? How many disks did each tserver have? Did they s Done -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Hao Hao has posted comments on this change. Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: PS5, Line 390: > See comment in log_block_manager.cc regarding this metric, and below I think this one is in the right place, only updated for SyncMetadata below. http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 886: RETURN_NOT_OK(SyncData()); > hmm. Should these metric increments happen inside SyncData() so they remain Makes sense. Updated. Line 1870: MakeContainerAvailableUnlocked(container); > Should also track this, no? Done http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/tserver/tablet_copy_client-test.cc File src/kudu/tserver/tablet_copy_client-test.cc: Line 91: metric_entity_ = METRIC_ENTITY_server.Instantiate(&metric_registry_, "test"); > I'm surprised it's OK for the registry to go out of scope and be destroyed Right... Thanks for catching it. http://gerrit.cloudera.org:8080/#/c/7701/5/src/kudu/tserver/tablet_copy_client.h File src/kudu/tserver/tablet_copy_client.h: PS5, Line 178: Data block is opened with new ID. > I don't understand what this remnant is trying to convey. Yeah, actually I do not quite follow the original comment. But let me try to rephrase it based on my understanding. -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] KUDU-1726: Avoid fsync-per-block in tablet copy
Hello Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7701 to look at the new patch set (#6). Change subject: KUDU-1726: Avoid fsync-per-block in tablet copy .. KUDU-1726: Avoid fsync-per-block in tablet copy This patch incorporates BlockTransaction API into tablet copy, to avoid fsync() on each block during copying. Blocks are synced to disk together once the tablet copy is complete. It also introduces a new block manager metric 'total_disk_sync' to keep track of block data disk synchronization count. I did a manual test to copy tablet with size of ~37GB from one tablet server to another on el6 with ext4. Each tablet server has a single disk. 'kudu remote_replica copy c53ffbc2ede84b6d9af2da607024d131 localhost:3334 localhost:3335' With this change, the operation time dropped down from ~718s to ~523s. Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 --- M src/kudu/fs/block_manager_metrics.cc M src/kudu/fs/block_manager_metrics.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/log_block_manager.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h 7 files changed, 68 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/7701/6 -- To view, visit http://gerrit.cloudera.org:8080/7701 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I7534699f589c7060ffe32d7ac67546476cf21e76 Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2098. Drop Spark 1 Support
Grant Henke has posted comments on this change. Change subject: KUDU-2098. Drop Spark 1 Support .. Patch Set 1: Yeah, I think that makes sense. -- To view, visit http://gerrit.cloudera.org:8080/7690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5481cc0477f4d23d89b68ef510a6c9a2aa187537 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] gradle: target JRE 7 compatibility
Grant Henke has posted comments on this change. Change subject: gradle: target JRE 7 compatibility .. Patch Set 2: Would +2 if I could. :D -- To view, visit http://gerrit.cloudera.org:8080/7785 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] gradle: target JRE 7 compatibility
Grant Henke has posted comments on this change. Change subject: gradle: target JRE 7 compatibility .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7785 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Adar Dembo has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 33: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 33 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 33: (3 comments) http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/block_manager.cc File src/kudu/fs/block_manager.cc: PS32, Line 50: close > closed Done http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 132 > OK sounds good to me, it can stay as is. I did some minor refactor to bring down the downcast times. http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS32, Line 1217: if (container_->read_only()) { : if (state_ != CLOSED) { : state_ = CLOSED; : if (container_->metrics()) { : container_->metrics()->generic_metrics.blocks_open_writing->Decrement(); : container_->metrics()->generic_metrics.total_bytes_written->IncrementBy( : block_length_); : } : > I would prefer we didn't change API semantics at this point. Multiple Abort Ok, sounds good. Updated. -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 33 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7207 to look at the new patch set (#33). Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. KUDU-1943: Add BlockTransaction to Block Manager This adds a new layer of abstraction 'BlockTransaction' to Block Manager, to accumulate new blocks in a logical operation, e.g. flush/compaction. Avoid using fsync() per block can potentionaly reduce a lot I/O overhead. This patch also add a new API, Finalize(), to Block Manager, so that an LBM container can be reused without flushing in-flight writable blocks to disk. This can bring down the log block container number and disk space consumption for first few flush/compaction operations when using log block manager. I performed manual test which creates a table with two tablets, 'kudu perf loadgen localhost --num-rows-per-thread=2000 --num-threads=10 --keep-auto-table --table_num_buckets=2'. With this patch, 'log_block_manager_containers' dropped from 56 to 7. A design doc can be found here: https://docs.google.com/document/d/1cB8pDs-Ho7p2HlzkWo1d-wmXr90YntIjdvdOv7u7gn0/edit?usp=sharing Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd --- M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.cc M src/kudu/fs/block_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/multi_column_writer.cc M src/kudu/tablet/multi_column_writer.h 20 files changed, 602 insertions(+), 437 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/7207/33 -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 33 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] Bump test timeout for flex partitioning-itest
Adar Dembo has posted comments on this change. Change subject: Bump test timeout for flex_partitioning-itest .. Patch Set 1: (3 comments) Test failure is weird; perhaps an oom kill on the Gradle daemon? http://gerrit.cloudera.org:8080/#/c/7814/1/CMakeLists.txt File CMakeLists.txt: Line 673: set(ARG_TIMEOUT 900) Can we avoid defining this default value both here and in run-test.sh? Given that we want to support running tests at multiple levels (running the binary directly, running it through run-test.sh, running it through ctest, running it through dist-test) this may not be possible. If you don't use the ctest timeout, you could condition all of the environment changes on ARG_TIMEOUT, which means the test timeout will be some custom value (if set in CMakeLists.txt) or the default value from run-test.sh (if not). Then there won't be a need to define this here. Line 707: # Set the ctest timeout to be a bit longer than the timeout we pass to Why bother with the ctest timeout at all, given that the run-test.sh timeout (respected by every test, dist-test or otherwise) is shorter? Are you worried about deadlocks/hangs induced by run-test.sh's post-processing? Line 714: if("${CUR_TEST_ENV}" STREQUAL "NOTFOUND") does if(NOT CUR_TEST_ENV) work? -- To view, visit http://gerrit.cloudera.org:8080/7814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Dan Burkert has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 32: (1 comment) http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 132 > There's no avoiding a downcast in LogBlockManager::CloseBlocks; we need it OK sounds good to me, it can stay as is. -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 32 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] [security] avoid kerberos ticket renewal and only reacquire
Todd Lipcon has posted comments on this change. Change subject: [security] avoid kerberos ticket renewal and only reacquire .. Patch Set 1: I think there is mention of 'renew' in init.h. Also I think we can remove the renew_lifetime handling in mini_kdc.{h,cc} as well as security-faults-itest.cc. -- To view, visit http://gerrit.cloudera.org:8080/7810 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8e5225de332ba785e3a73014b8418cfd4059fe07 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java] KUDU-2103 Canonicalize hostnames in client
Attila Bukor has posted comments on this change. Change subject: [java] KUDU-2103 Canonicalize hostnames in client .. Patch Set 6: You're welcome, thanks for merging it in. I've backported it to 1.3.x and 1.4.x, tested it locally using a Kerberized cluster, both versions worked fine. -- To view, visit http://gerrit.cloudera.org:8080/7757 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet
Adar Dembo has posted comments on this change. Change subject: Fix flakiness of kudu-admin-test TestMoveTablet .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7808 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR](branch-1.3.x) [java] KUDU-2103 Canonicalize hostnames in client
Attila Bukor has uploaded a new change for review. http://gerrit.cloudera.org:8080/7815 Change subject: [java] KUDU-2103 Canonicalize hostnames in client .. [java] KUDU-2103 Canonicalize hostnames in client When Kerberos is used the service principal contains the hostname of the service. This means that if a user wants to use an alias for the master_addresses in a Kerberized environment it won't be able to connect as the service principal name is not found in the Kerberos database. This patch adds canonicalization for the hostnames to make sure the principal name the service ticket is requested for matches the one used by the master servers. Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f --- M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.java M java/kudu-client/src/test/java/org/apache/kudu/client/FakeDNS.java A java/kudu-client/src/test/java/org/apache/kudu/client/TestServerInfo.java 4 files changed, 124 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/7815/1 -- To view, visit http://gerrit.cloudera.org:8080/7815 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: branch-1.3.x Gerrit-Owner: Attila Bukor
[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet
Alexey Serbin has posted comments on this change. Change subject: Fix flakiness of kudu-admin-test TestMoveTablet .. Patch Set 2: > Going to ignore IWYU since I dont think I touched anything that > should affect includes. You touched the raft_consensus.cc file, which was not IWYU compliant due to prior updates. I fixed that in f9a6a1b2d15feb8e2c044f20248774105a3a62bf. -- To view, visit http://gerrit.cloudera.org:8080/7808 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet
Hello Adar Dembo, Will Berkeley, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7808 to look at the new patch set (#2). Change subject: Fix flakiness of kudu-admin-test TestMoveTablet .. Fix flakiness of kudu-admin-test TestMoveTablet This fixes an issue in the admin tool for moving a tablet that would occur if the tablet changed terms multiple times before settling during an election. Previously, we would wait for a term change, and as soon as we saw any term change, we'd wait for an operation in exactly that term to be committed. There were two issues with the code: 1) It used cstate.has_leader_uuid() to check if there was a leader in the new term. However, the server side actually sets this field to an empty string rather than leaving it unset if there is no leader. So this check always evaluated to true, meaning that we would proceed to looking for a committed op in that term as soon as the term advanced, rather than waiting for an actual leader to be elected. 2) In the case that somehow the leader changed a second time, the term could be increased again while we are waiting for a committed operation. In that case we'd be waiting for a committed op in exactly the term of the first leader change we saw, rather than potentially refreshing our notion of the "current term". The patch fixes both issues by restructuring the loop a bit. I additionally improved some logging in the consensus service and implementation which I found helpful while debugging the issue. This test became significantly more flaky (~20% in ASAN) after the commit of 21b0f3d5e255760535e281efe5879fe657df1f1c which adjusted the election timeout behavior. Apparently the new election behavior made it more likely for the elections to have conflicts before getting a steady leader, which exposed the above bug in the tool. With the patch, I was able to run 500/500 successful in an ASAN build (vs 20% fail rate before). Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177 --- M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/tools/tool_action_tablet.cc M src/kudu/tserver/tablet_service.cc 4 files changed, 54 insertions(+), 48 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/7808/2 -- To view, visit http://gerrit.cloudera.org:8080/7808 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet
Todd Lipcon has posted comments on this change. Change subject: Fix flakiness of kudu-admin-test TestMoveTablet .. Patch Set 1: Verified+1 Going to ignore IWYU since I dont think I touched anything that should affect includes. -- To view, visit http://gerrit.cloudera.org:8080/7808 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] Bump test timeout for flex partitioning-itest
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7814 to review the following change. Change subject: Bump test timeout for flex_partitioning-itest .. Bump test timeout for flex_partitioning-itest In precommit runs, this test is sharded 8 ways so it never has issues with timeouts. However, in some Jenkins builds that don't use dist-test, it can time out, especially when TSAN is enabled. This adds functionality to our ADD_KUDU_TEST() cmake function to specify a timeout and plumbs that timeout through to our test wrapper shell script. Tested by verifying that the appropriate environment variable gets set in the environment of the running test. Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 --- M CMakeLists.txt M src/kudu/integration-tests/CMakeLists.txt 2 files changed, 40 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/7814/1 -- To view, visit http://gerrit.cloudera.org:8080/7814 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo
[kudu-CR] [rpc] faster generation of KRPC call ID
Todd Lipcon has posted comments on this change. Change subject: [rpc] faster generation of KRPC call ID .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7813/2//COMMIT_MSG Commit Message: PS2, Line 48: const int u_bound = 8 + (random() % 100); : int n = 0; : for (int i = 0; i < u_bound; ++i) { : n = next_id(); : } > Yep, I suspected that it's not a good benchmark, but the newer code looked yea, if you compile in clang it turns it into a constant time operation. g++ on my machine isn't quite smart enough but it seems it is on yours, perhaps. Adding the noinline attribute on the function breaks that optimization of course and it goes back to being the same speed. Either way, I think it's a "if it aint broke dont fix it" scenario IMO -- To view, visit http://gerrit.cloudera.org:8080/7813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [rpc] faster generation of KRPC call ID
Alexey Serbin has posted comments on this change. Change subject: [rpc] faster generation of KRPC call ID .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7813/2//COMMIT_MSG Commit Message: PS2, Line 48: const int u_bound = 8 + (random() % 100); : int n = 0; : for (int i = 0; i < u_bound; ++i) { : n = next_id(); : } > I think your microbenchmark is invalid. With -O3 it compiles to this assemb Yep, I suspected that it's not a good benchmark, but the newer code looked better IMO. -- To view, visit http://gerrit.cloudera.org:8080/7813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [rpc] faster generation of KRPC call ID
Todd Lipcon has posted comments on this change. Change subject: [rpc] faster generation of KRPC call ID .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/7813/2//COMMIT_MSG Commit Message: PS2, Line 48: const int u_bound = 8 + (random() % 100); : int n = 0; : for (int i = 0; i < u_bound; ++i) { : n = next_id(); : } > I think your microbenchmark is invalid. With -O3 it compiles to this assemb oh wait, I may have misread the assembly output... you can ignore this. But still I'm skeptical of the benchmark here. -- To view, visit http://gerrit.cloudera.org:8080/7813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [rpc] faster generation of KRPC call ID
Todd Lipcon has posted comments on this change. Change subject: [rpc] faster generation of KRPC call ID .. Patch Set 2: Code-Review-1 (1 comment) don't think this is a worthwhile optimization since this isn't a hot code path http://gerrit.cloudera.org:8080/#/c/7813/2//COMMIT_MSG Commit Message: PS2, Line 48: const int u_bound = 8 + (random() % 100); : int n = 0; : for (int i = 0; i < u_bound; ++i) { : n = next_id(); : } I think your microbenchmark is invalid. With -O3 it compiles to this assembly: int main() { 4007a0: 48 83 ec 08 sub$0x8,%rsp srandom(time(nullptr)); 4007a4: 31 ff xor%edi,%edi 4007a6: e8 d5 ff ff ff callq 400780 4007ab: 89 c7 mov%eax,%edi 4007ad: e8 8e ff ff ff callq 400740 const int u_bound = 8 + (random() % 100); 4007b2: e8 a9 ff ff ff callq 400760 4007b7: 48 ba 0b d7 a3 70 3dmovabs $0xa3d70a3d70a3d70b,%rdx 4007be: 0a d7 a3 4007c1: 48 89 c1mov%rax,%rcx 4007c4: 48 f7 eaimul %rdx 4007c7: 48 89 c8mov%rcx,%rax 4007ca: 48 c1 f8 3f sar$0x3f,%rax 4007ce: 48 01 caadd%rcx,%rdx 4007d1: 48 c1 fa 06 sar$0x6,%rdx 4007d5: 48 29 c2sub%rax,%rdx 4007d8: 48 8d 04 92 lea(%rdx,%rdx,4),%rax ie it's turning the loop into a single constant time multiplication. -- To view, visit http://gerrit.cloudera.org:8080/7813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [rpc] faster generation of KRPC call ID
Alexey Serbin has uploaded a new patch set (#2). Change subject: [rpc] faster generation of KRPC call ID .. [rpc] faster generation of KRPC call ID Updated the way how the KRPC call identifiers are generated. To compare, I used the following code (headers are omitted): old.cc: - int32_t cur_id = 0; int32_t next_id() { if (PREDICT_FALSE(cur_id == std::numeric_limits::max())) { cur_id = 0; } else { ++cur_id; } return cur_id; } int main() { srandom(time(nullptr)); const int u_bound = 8 + (random() % 100); int n = 0; for (int i = 0; i < u_bound; ++i) { n = next_id(); } cout << n << endl; return 0; } - new.cc: - int32_t cur_id = 0; int32_t next_id() { ++cur_id; return (cur_id & 0x7fff); } int main() { srandom(time(nullptr)); const int u_bound = 8 + (random() % 100); int n = 0; for (int i = 0; i < u_bound; ++i) { n = next_id(); } std::cout << n << std::endl; return 0; } - My ad-hoc measurements show that the new version is faster even if compiling both with -O3 optimization flag: old: real 0m0.831s user 0m0.822s sys 0m0.005s new: real 0m0.005s user 0m0.001s sys 0m0.002s >From the 'perf stat' perspective it looks good as well: - Performance counter stats for './old' (10 runs): 790.584545 task-clock#1.001 CPUs utilized 1 context-switches #0.002 K/sec 1 cpu-migrations#0.001 K/sec 273 page-faults #0.345 K/sec 2,583,562,591 cycles#3.268 GHz 2,805,308,074 instructions #1.09 insns per cycle 200,917,139 branches # 254.137 M/sec 13,513 branch-misses #0.01% of all branches 0.790023494 seconds time elapsed - - Performance counter stats for './new' (10 runs): 0.888215 task-clock#0.831 CPUs utilized 0 context-switches #0.000 K/sec 0 cpu-migrations#0.000 K/sec 273 page-faults #0.307 M/sec 2,561,042 cycles#2.883 GHz 2,804,468 instructions #1.10 insns per cycle 480,394 branches # 540.853 M/sec 11,575 branch-misses #2.41% of all branches 0.001068744 seconds time elapsed - Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6 --- M src/kudu/rpc/connection.cc M src/kudu/rpc/connection.h 2 files changed, 9 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/7813/2 -- To view, visit http://gerrit.cloudera.org:8080/7813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [rpc] faster generation of KRPC call ID
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/7813 Change subject: [rpc] faster generation of KRPC call ID .. [rpc] faster generation of KRPC call ID Updated the way how the KRPC call identifiers are generated. To compare, I used the following code (headers are omitted): old.cc: - int32_t cur_id = 0; int32_t next_id() { if (PREDICT_FALSE(cur_id == std::numeric_limits::max())) { cur_id = 0; } else { ++cur_id; } return cur_id; } int main() { srandom(time(nullptr)); const int u_bound = 8 + (random() % 100); int n = 0; for (int i = 0; i < u_bound; ++i) { n = next_id(); } cout << n << endl; return 0; } - new.cc: - int32_t cur_id = 0; int32_t next_id() { ++cur_id; return (cur_id & 0x7fff); } int main() { srandom(time(nullptr)); const int u_bound = 8 + (random() % 100); int n = 0; for (int i = 0; i < u_bound; ++i) { n = next_id(); } std::cout << n << std::endl; return 0; } - My ad-hoc measurements show that the new version is faster even if compiling both with -O3 optimization flag: old: real 0m0.831s user 0m0.822s sys 0m0.005s new: real 0m0.005s user 0m0.001s sys 0m0.002s >From the 'perf stat' perspective it looks good as well: - Performance counter stats for './old' (10 runs): 790.584545 task-clock#1.001 CPUs utilized 1 context-switches #0.002 K/sec 1 cpu-migrations#0.001 K/sec 273 page-faults #0.345 K/sec 2,583,562,591 cycles#3.268 GHz 2,805,308,074 instructions #1.09 insns per cycle 200,917,139 branches # 254.137 M/sec 13,513 branch-misses #0.01% of all branches 0.790023494 seconds time elapsed - - Performance counter stats for './new' (10 runs): 0.888215 task-clock#0.831 CPUs utilized 0 context-switches #0.000 K/sec 0 cpu-migrations#0.000 K/sec 273 page-faults #0.307 M/sec 2,561,042 cycles#2.883 GHz 2,804,468 instructions #1.10 insns per cycle 480,394 branches # 540.853 M/sec 11,575 branch-misses #2.41% of all branches 0.001068744 seconds time elapsed - Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6 --- M src/kudu/rpc/connection.h 1 file changed, 7 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/7813/1 -- To view, visit http://gerrit.cloudera.org:8080/7813 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I03726343d222bcd241c2c2a5a1670a672f8e5cb6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
[kudu-CR] [iwyu] another catch-up update
Alexey Serbin has posted comments on this change. Change subject: [iwyu] another catch-up update .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7802/3/src/kudu/rpc/periodic.cc File src/kudu/rpc/periodic.cc: Line 24: #include > Makes sense, thanks. Thank you for prompt and thoughtful review! -- To view, visit http://gerrit.cloudera.org:8080/7802 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR](branch-1.4.x) [java] KUDU-2103 Canonicalize hostnames in client
Attila Bukor has uploaded a new change for review. http://gerrit.cloudera.org:8080/7812 Change subject: [java] KUDU-2103 Canonicalize hostnames in client .. [java] KUDU-2103 Canonicalize hostnames in client When Kerberos is used the service principal contains the hostname of the service. This means that if a user wants to use an alias for the master_addresses in a Kerberized environment it won't be able to connect as the service principal name is not found in the Kerberos database. This patch adds canonicalization for the hostnames to make sure the principal name the service ticket is requested for matches the one used by the master servers. Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f --- M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.java M java/kudu-client/src/main/java/org/apache/kudu/client/TabletClient.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/TestServerInfo.java 4 files changed, 89 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/7812/1 -- To view, visit http://gerrit.cloudera.org:8080/7812 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: branch-1.4.x Gerrit-Owner: Attila Bukor
[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet
Todd Lipcon has posted comments on this change. Change subject: Fix flakiness of kudu-admin-test TestMoveTablet .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7808/1//COMMIT_MSG Commit Message: PS1, Line 36: Apparently the new election behavior made it more likely : for the elections to have conflicts before getting a steady leader > That seems like a problem too. From your debugging were you able to pinpoin no, wasn't entirely clear. I figured it was the timing changes you made in your patch (bisect clearly pointed to the above-referenced commit as where it got worse). -- To view, visit http://gerrit.cloudera.org:8080/7808 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] [iwyu] another catch-up update
Adar Dembo has posted comments on this change. Change subject: [iwyu] another catch-up update .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7802/3/src/kudu/rpc/periodic.cc File src/kudu/rpc/periodic.cc: Line 24: #include > As I understand, this file uses Messenger::ScheduleOnReactor() method, whic Makes sense, thanks. -- To view, visit http://gerrit.cloudera.org:8080/7802 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [iwyu] another catch-up update
Alexey Serbin has submitted this change and it was merged. Change subject: [iwyu] another catch-up update .. [iwyu] another catch-up update This is just another catch-up patch after recent updates: since the IWYU gerrit was not enabled for a while, some files became not compliant with IWYU. This patch fixes that, keeping the code in IWYU-compliant state. I also sneaked in a few updates on other files to remove them from the 'muted' list in build-support/iwyu/iwyu-filter.awk Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1 Reviewed-on: http://gerrit.cloudera.org:8080/7802 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo --- M build-support/iwyu/iwyu-filter.awk M src/kudu/cfile/index_btree.cc M src/kudu/cfile/mt-bloomfile-test.cc M src/kudu/clock/hybrid_clock-test.cc M src/kudu/clock/logical_clock-test.cc M src/kudu/codegen/codegen-test.cc M src/kudu/common/partition-test.cc M src/kudu/common/partition_pruner-test.cc M src/kudu/consensus/consensus_meta_manager-stress-test.cc M src/kudu/consensus/consensus_peers.cc M src/kudu/consensus/consensus_peers.h M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/block_manager_util-test.cc M src/kudu/fs/block_manager_util.cc M src/kudu/fs/block_manager_util.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/integration-tests/disk_failure-itest.cc M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/rpc/connection.h M src/kudu/rpc/negotiation-test.cc M src/kudu/rpc/negotiation.cc M src/kudu/rpc/outbound_call.h M src/kudu/rpc/periodic-test.cc M src/kudu/rpc/periodic.cc M src/kudu/rpc/periodic.h M src/kudu/rpc/reactor.cc M src/kudu/rpc/reactor.h M src/kudu/rpc/rpc-test.cc M src/kudu/rpc/rpc_stub-test.cc M src/kudu/rpc/transfer.cc M src/kudu/rpc/transfer.h M src/kudu/security/ca/cert_management-test.cc M src/kudu/security/cert-test.cc M src/kudu/security/crypto-test.cc M src/kudu/security/tls_handshake-test.cc M src/kudu/security/token-test.cc M src/kudu/server/rpc_server-test.cc M src/kudu/tablet/all_types-scan-correctness-test.cc M src/kudu/tablet/deltafile.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/memrowset.h M src/kudu/tablet/mt-rowset_delta_compaction-test.cc M src/kudu/tablet/mt-tablet-test.cc M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica.h M src/kudu/tablet/transactions/transaction_tracker-test.cc M src/kudu/tools/tool_action.cc M src/kudu/tools/tool_action_master.cc M src/kudu/tools/tool_action_tablet.cc M src/kudu/tools/tool_action_tserver.cc M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/util/compression/compression-test.cc M src/kudu/util/file_cache-stress-test.cc M src/kudu/util/subprocess.cc 68 files changed, 222 insertions(+), 143 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7802 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [iwyu] another catch-up update
Alexey Serbin has posted comments on this change. Change subject: [iwyu] another catch-up update .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7802/3/src/kudu/rpc/periodic.cc File src/kudu/rpc/periodic.cc: Line 24: #include > I wrote this code and I'm curious why this is needed. There are no boost::f As I understand, this file uses Messenger::ScheduleOnReactor() method, which takes boost::function as its first parameter. If I understand correctly, to construct an object of that type, the information on the boost::function is needed for the compiler. -- To view, visit http://gerrit.cloudera.org:8080/7802 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [security] avoid kerberos ticket renewal and only reacquire
Sailesh Mukil has uploaded a new change for review. http://gerrit.cloudera.org:8080/7810 Change subject: [security] avoid kerberos ticket renewal and only reacquire .. [security] avoid kerberos ticket renewal and only reacquire It was found that if we use a file based credential cache that is shared between the C++ side and the java side of a process, and we encounter the specific edge case where we renew a ticket that has less than 'ticket_lifetime' left before its 'renew_lifetime' expires, the ticket is set to have a NULL 'renew_till' timestamp. Eg: ticket_lifetime = 10m renew_lifetime = 100m [current ticket being renewed] at '15:30:00' endtime = '15:30:30' renew_till = '15:31:00' This ticket will be renewed and the renewed ticket will have the following values: endtime = '15:31:00' renew_till = null The Java krb5 library refuses to read these kinds of tickets which have the RENEWABLE flag set but no 'renew_till' set, causing unexpected failures. Currently, the only way to work around this is to not renew tickets at all and only always reacquire them. The reason for this is that the Java side of a process or even another process may be running its own renewal thread on the same credential cache for the same principal(s). So even if we were to avoid renewing in this window, the Java side could renew in this window, causing the above problem. If we always reacquire the tickets, we're forcefully reseting this window for that principal, thereby not allowing the Java side to hit this bug. The scenario where this bug played out is when using the kudu renewal code in tandem with a hadoop process that use the same principals. Also, currently there is no advantage we gain from just renewing the tickets vs. reacquiring them, either in terms of security or performance, since we login from a keytab. Tracked on the Java side by: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8186576 Change-Id: I8e5225de332ba785e3a73014b8418cfd4059fe07 --- M src/kudu/integration-tests/external_mini_cluster-test.cc M src/kudu/security/init.cc 2 files changed, 19 insertions(+), 60 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/7810/1 -- To view, visit http://gerrit.cloudera.org:8080/7810 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I8e5225de332ba785e3a73014b8418cfd4059fe07 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil
[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet
Will Berkeley has posted comments on this change. Change subject: Fix flakiness of kudu-admin-test TestMoveTablet .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7808 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: No
[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet
Adar Dembo has posted comments on this change. Change subject: Fix flakiness of kudu-admin-test TestMoveTablet .. Patch Set 1: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/7808/1//COMMIT_MSG Commit Message: PS1, Line 36: Apparently the new election behavior made it more likely : for the elections to have conflicts before getting a steady leader That seems like a problem too. From your debugging were you able to pinpoint why? -- To view, visit http://gerrit.cloudera.org:8080/7808 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] [iwyu] another catch-up update
Adar Dembo has posted comments on this change. Change subject: [iwyu] another catch-up update .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/7802/3/src/kudu/rpc/periodic.cc File src/kudu/rpc/periodic.cc: Line 24: #include I wrote this code and I'm curious why this is needed. There are no boost::function instances here. Do you know what's going on? -- To view, visit http://gerrit.cloudera.org:8080/7802 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1690e59e8756732db9d23c1506be9f3aa2272ec1 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2098. Drop Spark 1 Support
Dan Burkert has posted comments on this change. Change subject: KUDU-2098. Drop Spark 1 Support .. Patch Set 1: So is the plan that this can merge after 1.5 branches? -- To view, visit http://gerrit.cloudera.org:8080/7690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5481cc0477f4d23d89b68ef510a6c9a2aa187537 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Adar Dembo has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 32: (2 comments) http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 132 > Adar earlier made the point "it shouldn't be part of the WritableBlock inte There's no avoiding a downcast in LogBlockManager::CloseBlocks; we need it in order to call other non-interface methods on LogWritableBlock. But, if you prefer to reduce the number of downcasts from two to one, some light refactoring could do that. http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS32, Line 1217: if (container_->read_only()) { : state_ = CLOSED; : if (container_->metrics()) { : container_->metrics()->generic_metrics.blocks_open_writing->Decrement(); : container_->metrics()->generic_metrics.total_bytes_written->IncrementBy( : block_length_); : } : return Status::Aborted("container $0 is read-only", container_->ToString()); : } > Makes sense. I don't see a case we would want to Abort() the block if it is I would prefer we didn't change API semantics at this point. Multiple Abort() calls in succession are legal; let's keep them that way (and ensure they no-op). -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 32 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] gradle: target JRE 7 compatibility
Todd Lipcon has posted comments on this change. Change subject: gradle: target JRE 7 compatibility .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7785 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [security] avoid tickets with NULL 'renew till'
Sailesh Mukil has abandoned this change. Change subject: [security] avoid tickets with NULL 'renew_till' .. Abandoned Turns out that this patch makes the issue mentioned in the commit message less severe, but still hits it occasionally. The reason is that if the Kudu side renew tickets, the Java side (from hadoop-common in my testing) could also have its own renewal thread working on the same credential cache. And even if we're careful with our renewals, the java side could renew outside the ideal window that we're trying to maintain. The only other way to work around this is to always reacquire instead of renew, thereby not giving a chance for the Java side renewal thread to renew outside this ideal window, as our reacquisition of the ticket would always reset the window for the new ticket in the credential cache. Also, we don't have anything to gain in terms of performance or security from renewing vs. reacquiring when we login from a keytab. I will put out another patch which removes the renewal code and only always reacquires. -- To view, visit http://gerrit.cloudera.org:8080/7770 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [security] avoid tickets with NULL 'renew till'
Sailesh Mukil has posted comments on this change. Change subject: [security] avoid tickets with NULL 'renew_till' .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7770/2/src/kudu/security/init.cc File src/kudu/security/init.cc: PS2, Line 295: > I might be missing some crucial point here, but if renew_deadline is negati You're right. That was my bad. In any case, I have to abandon this patch. After quite a lot of testing, it shows that this still doesn't work around the problem as I previously thought. I've added a top level review comment as to why it doesn't work. -- To view, visit http://gerrit.cloudera.org:8080/7770 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I59194af94838f680df4ce121a8dee526a876e369 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sailesh Mukil Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Fix flakiness of kudu-admin-test TestMoveTablet
Hello Adar Dembo, Will Berkeley, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7808 to review the following change. Change subject: Fix flakiness of kudu-admin-test TestMoveTablet .. Fix flakiness of kudu-admin-test TestMoveTablet This fixes an issue in the admin tool for moving a tablet that would occur if the tablet changed terms multiple times before settling during an election. Previously, we would wait for a term change, and as soon as we saw any term change, we'd wait for an operation in exactly that term to be committed. There were two issues with the code: 1) It used cstate.has_leader_uuid() to check if there was a leader in the new term. However, the server side actually sets this field to an empty string rather than leaving it unset if there is no leader. So this check always evaluated to true, meaning that we would proceed to looking for a committed op in that term as soon as the term advanced, rather than waiting for an actual leader to be elected. 2) In the case that somehow the leader changed a second time, the term could be increased again while we are waiting for a committed operation. In that case we'd be waiting for a committed op in exactly the term of the first leader change we saw, rather than potentially refreshing our notion of the "current term". The patch fixes both issues by restructuring the loop a bit. I additionally improved some logging in the consensus service and implementation which I found helpful while debugging the issue. This test became significantly more flaky (~20% in ASAN) after the commit of 21b0f3d5e255760535e281efe5879fe657df1f1c which adjusted the election timeout behavior. Apparently the new election behavior made it more likely for the elections to have conflicts before getting a steady leader, which exposed the above bug in the tool. With the patch, I was able to run 500/500 successful in an ASAN build (vs 20% fail rate before). Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177 --- M src/kudu/consensus/raft_consensus.cc M src/kudu/consensus/raft_consensus.h M src/kudu/tools/tool_action_tablet.cc M src/kudu/tserver/tablet_service.cc 4 files changed, 54 insertions(+), 48 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/7808/1 -- To view, visit http://gerrit.cloudera.org:8080/7808 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I475d4a44c52f2da7fc42c93e9cf2f38e01735177 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-1894 fixed deadlock in client.Connection
Dan Burkert has posted comments on this change. Change subject: KUDU-1894 fixed deadlock in client.Connection .. Patch Set 4: This looks good to me, but obviously this code is extremely fragile. We should figure out a way to make this more robust down the line. I took a quick look at the server implementation and it seems like it would be OK with out of order call IDs. -- To view, visit http://gerrit.cloudera.org:8080/7765 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a0edc3e3accbcff60f2cde641ee470312bbd27a Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 20: (2 comments) http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 132 > Looks like this is being retained as a method on both implementations, but Adar earlier made the point "it shouldn't be part of the WritableBlock interface because there's no need for anyone outside the block manager to know about the distinction." It makes sense to me from a interface point of view. I prefer to keep the change (which removed the interface) unless you have a strong preference? http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS32, Line 1217: : // DoCloseBlocks() has unlocked the container; it may be locked by someone else. : // But block_manager_ is immutable, so this is safe. : return container_->block_manager()->DeleteBlock(id()); : } : : const BlockId& LogWritableBlock::id() const { : return block_id_; : } > Abort() is part of the public API of a WritableBlock, so theoretically, any Makes sense. I don't see a case we would want to Abort() the block if it is successfully closed for now (could be I miss something?). So I prefer to do CHECK on the state. -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 20 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] gradle: target JRE 7 compatibility
Dan Burkert has posted comments on this change. Change subject: gradle: target JRE 7 compatibility .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7785/1/java/gradle/compile.gradle File java/gradle/compile.gradle: Line 23: targetCompatibility = javaSourceCompatibility > Since source and target versions will likely always match perhaps we should Done -- To view, visit http://gerrit.cloudera.org:8080/7785 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] gradle: target JRE 7 compatibility
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7785 to look at the new patch set (#2). Change subject: gradle: target JRE 7 compatibility .. gradle: target JRE 7 compatibility Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29 --- M java/gradle.properties M java/gradle/compile.gradle 2 files changed, 7 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/7785/2 -- To view, visit http://gerrit.cloudera.org:8080/7785 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib836a748f0f000b07460dd3141940655dbfb7a29 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1894 fixed deadlock in client.Connection
Alexey Serbin has posted comments on this change. Change subject: KUDU-1894 fixed deadlock in client.Connection .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/7765/4/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java: Line 524: sendCallToWire(msg); > yea but call_id is an int64 so it would require a single TCP connection to As far as I could see, call_id is int32_t, so I would expect it would rotate much faster :) Yes, sure -- I agree it's better to be careful here. I took a look at the code and I didn't find any place where non-ascending call_id (or sparse sequence of those) could break the code. However, I might be missing something -- it's better to be safe than sorry. -- To view, visit http://gerrit.cloudera.org:8080/7765 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a0edc3e3accbcff60f2cde641ee470312bbd27a Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Dan Burkert has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 32: (1 comment) http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 132 Looks like this is being retained as a method on both implementations, but to call it requires a downcast. Should we keep it in the interface so that the downcast isn't necessary? -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 32 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Mike Percy has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 32: (1 comment) http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS32, Line 1217: if (container_->read_only()) { : state_ = CLOSED; : if (container_->metrics()) { : container_->metrics()->generic_metrics.blocks_open_writing->Decrement(); : container_->metrics()->generic_metrics.total_bytes_written->IncrementBy( : block_length_); : } : return Status::Aborted("container $0 is read-only", container_->ToString()); : } > Yeah, it does not hurt to add the checking. But looking at the flow, Abort( Abort() is part of the public API of a WritableBlock, so theoretically, anybody handling a block can abort it. But if we document that Abort() must not be called when a block is closed then I suppose we can just CHECK on it or something like that. Seems a little fragile, though. -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 32 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-1894 fixed deadlock in client.Connection
Todd Lipcon has posted comments on this change. Change subject: KUDU-1894 fixed deadlock in client.Connection .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/7765/4/java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java File java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java: Line 524: sendCallToWire(msg); > Another thought is that the sequence anyway could go non-ascending order wh yea but call_id is an int64 so it would require a single TCP connection to last for a few millennia for that to happen :) Let me take some time to ponder this patch a bit more. -- To view, visit http://gerrit.cloudera.org:8080/7765 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1a0edc3e3accbcff60f2cde641ee470312bbd27a Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-1943: Add BlockTransaction to Block Manager
Hao Hao has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager .. Patch Set 32: (1 comment) http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS32, Line 1217: if (container_->read_only()) { : state_ = CLOSED; : if (container_->metrics()) { : container_->metrics()->generic_metrics.blocks_open_writing->Decrement(); : container_->metrics()->generic_metrics.total_bytes_written->IncrementBy( : block_length_); : } : return Status::Aborted("container $0 is read-only", container_->ToString()); : } > It's hard to tell whether it's possible to wind up calling Abort() twice on Yeah, it does not hurt to add the checking. But looking at the flow, Abort() is only called when the state_ != CLOSED now. So instead maybe I can add a DCHECK of the state_ at the beginning of Abort()? -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a4bd Gerrit-PatchSet: 32 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] [java] KUDU-2103 Canonicalize hostnames in client
Todd Lipcon has posted comments on this change. Change subject: [java] KUDU-2103 Canonicalize hostnames in client .. Patch Set 6: Thanks for the fix, Attila. I think we should cherry pick this back to branch-1.4.x and branch-1.3.x, but it looks like it's not a clean cherry pick. Would you mind doing the backports? You can post to gerrit using 'git push gerrit HEAD:refs/for/branch-1.4.x' for example -- To view, visit http://gerrit.cloudera.org:8080/7757 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java] KUDU-2103 Canonicalize hostnames in client
Todd Lipcon has posted comments on this change. Change subject: [java] KUDU-2103 Canonicalize hostnames in client .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7757 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] [java] KUDU-2103 Canonicalize hostnames in client
Todd Lipcon has submitted this change and it was merged. Change subject: [java] KUDU-2103 Canonicalize hostnames in client .. [java] KUDU-2103 Canonicalize hostnames in client When Kerberos is used the service principal contains the hostname of the service. This means that if a user wants to use an alias for the master_addresses in a Kerberized environment it won't be able to connect as the service principal name is not found in the Kerberos database. This patch adds canonicalization for the hostnames to make sure the principal name the service ticket is requested for matches the one used by the master servers. Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f Reviewed-on: http://gerrit.cloudera.org:8080/7757 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M java/kudu-client/src/main/java/org/apache/kudu/client/Connection.java M java/kudu-client/src/main/java/org/apache/kudu/client/ServerInfo.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/TestServerInfo.java 4 files changed, 91 insertions(+), 6 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7757 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I77204a185c1ab8e21cc833afb645543cbc0e340f Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Attila Bukor Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR](branch-1.2.x) log block manager: use unsigned int for next block id
Todd Lipcon has submitted this change and it was merged. Change subject: log block manager: use unsigned int for next_block_id_ .. log block manager: use unsigned int for next_block_id_ KUDU-1538 introduced 'next_block_id_' to keep track of unique block ID that should be used for block creation. Currently, it is defined as int64_t. However, it could be updated based on the value of 'max_block_id' which is uint64_t. Since block IDs are defined as uint64_t both on disk (fs.proto) and in memory (block_id.h), it makes more sense to treat 'next_block_id_' as uint64_t rather than to convert it correctly to int64_t everywhere. This patch changes the type of 'next_block_id_' to uint64_t to avoid overflow due to conversion of unsigned int to int, which can result in the reuse of an existing block ID. It does not add a standalone test case because the failure is most likely to occur in specific test-only scenarios. Change-Id: Ib315b20719ef529331304df5c56c4242902524d4 Reviewed-on: http://gerrit.cloudera.org:8080/7796 Reviewed-by: Adar Dembo Reviewed-by: Todd Lipcon Tested-by: Kudu Jenkins (cherry picked from commit b37bde72c47370293827a306532d9be15b1c5f40) Reviewed-on: http://gerrit.cloudera.org:8080/7799 --- M src/kudu/fs/file_block_manager.h M src/kudu/fs/log_block_manager.h 2 files changed, 2 insertions(+), 2 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7799 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ib315b20719ef529331304df5c56c4242902524d4 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: branch-1.2.x Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR](branch-1.3.x) log block manager: use unsigned int for next block id
Todd Lipcon has submitted this change and it was merged. Change subject: log block manager: use unsigned int for next_block_id_ .. log block manager: use unsigned int for next_block_id_ KUDU-1538 introduced 'next_block_id_' to keep track of unique block ID that should be used for block creation. Currently, it is defined as int64_t. However, it could be updated based on the value of 'max_block_id' which is uint64_t. Since block IDs are defined as uint64_t both on disk (fs.proto) and in memory (block_id.h), it makes more sense to treat 'next_block_id_' as uint64_t rather than to convert it correctly to int64_t everywhere. This patch changes the type of 'next_block_id_' to uint64_t to avoid overflow due to conversion of unsigned int to int, which can result in the reuse of an existing block ID. It does not add a standalone test case because the failure is most likely to occur in specific test-only scenarios. Change-Id: Ib315b20719ef529331304df5c56c4242902524d4 Reviewed-on: http://gerrit.cloudera.org:8080/7796 Reviewed-by: Adar Dembo Reviewed-by: Todd Lipcon Tested-by: Kudu Jenkins (cherry picked from commit b37bde72c47370293827a306532d9be15b1c5f40) Reviewed-on: http://gerrit.cloudera.org:8080/7798 --- M src/kudu/fs/file_block_manager.h M src/kudu/fs/log_block_manager.h 2 files changed, 2 insertions(+), 2 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ib315b20719ef529331304df5c56c4242902524d4 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: branch-1.3.x Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon