[kudu-CR] [mini-cluster] allow 18-bit PID in LOOPBACK bind mode
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8422 to look at the new patch set (#4). Change subject: [mini-cluster] allow 18-bit PID in LOOPBACK bind mode .. [mini-cluster] allow 18-bit PID in LOOPBACK bind mode This changelist allows to use 18-bit PIDs for binding in LOOPBACK mode. Prior to this patch, the minicluster could not use PIDs wider than 16 bits. While expanding the range of acceptable PIDs, this changelist also decreases the maximum allowed number of 'indexed servers' from 254 to 62. As of now, that's enough for our minicluster-based tests. The incentive to put up this patch was change of the max PID up to 147456 in /proc/sys/kernel/pid_max after recent restart of one of our build servers. Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130 --- M src/kudu/mini-cluster/mini_cluster.cc 1 file changed, 20 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/8422/4 -- To view, visit http://gerrit.cloudera.org:8080/8422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130 Gerrit-Change-Number: 8422 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [mini-cluster] allow 18-bit PID in LOOPBACK bind mode
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8422 to look at the new patch set (#3). Change subject: [mini-cluster] allow 18-bit PID in LOOPBACK bind mode .. [mini-cluster] allow 18-bit PID in LOOPBACK bind mode This changelist allows to use 18-bit PID for binding in LOOPBACK mode. Prior to this patch, the minicluster supported PIDs to be up to 16 bits wide. While expanding the range for PIDs, this also narrows down the limit on maximum number of 'indexed servers' to 62 (it was 262 prior to this change). Hopefully, that's enough for our minicluster tests. The incentive to put up this patch was change of the max PID up to 147456 in /proc/sys/kernel/pid_max after recent restart of one of our build servers. Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130 --- M src/kudu/mini-cluster/mini_cluster.cc 1 file changed, 20 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/8422/3 -- To view, visit http://gerrit.cloudera.org:8080/8422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130 Gerrit-Change-Number: 8422 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [mini-cluster] allow 18-bit PID in LOOPBACK bind mode
Hello Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8422 to look at the new patch set (#2). Change subject: [mini-cluster] allow 18-bit PID in LOOPBACK bind mode .. [mini-cluster] allow 18-bit PID in LOOPBACK bind mode This changelist allows to use 18-bit PID for binding in LOOPBACK mode. Prior to this patch, the minicluster supported PIDs to be up to 16 bits wide. While expanding the range for PIDs, this also narrows down the limit on maximum number of 'indexed servers' to 62 (it was 262 prior to this change). Hopefully, that's enough for our minicluster tests. The incentive to put up this patch was change of the max PID up to 147456 in /proc/sys/kernel/pid_max after recent restart of one of our build servers. Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130 --- M src/kudu/mini-cluster/mini_cluster.cc 1 file changed, 20 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/8422/2 -- To view, visit http://gerrit.cloudera.org:8080/8422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130 Gerrit-Change-Number: 8422 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] cfile set: make FindRow more robust to errors
Andrew Wong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8423 Change subject: cfile_set: make FindRow more robust to errors .. cfile_set: make FindRow more robust to errors Previously, FindRow() would not handle certain error cases well. I.e. when seeking on a key, it wouldn't consider all errors before returning the seek results. For example, currently, FindRow() scans over CFiles and if 1) it hits a Status::NotFound() error, or 2) the key search returned without being an exact match, it returns success, returning the row is not present. However, 2) does not take into account the OK-ness of the returned scans, and thus may return healthily saying it couldn't find the row, even though this may not be the case. This allows the currently-running operation to continue under the potentially incorrect assumption that the key is not present, when it should actually return with the appropriate error. In the case of DuplicatingRowSet::MutateRow(), this is particularly nasty: it expects operations to be mirrored to two separate rowsets, and the above scenario may present itself as a fatal failure to do so. In practice, this hasn't been an issue because we might only expect various fs-level errors (e.g. corruption, disk errors) here, which we already don't handle gracefully. However, this needs to change to be robust to disk failures. Similarly, FindRow() will now return early if it hits a disk failure in the bloom look-up. A small test is added to diskrowset-test to ensure errors are returned appropriately. Change-Id: I41b56cbbf1b3b00fdb4dbf1ec08f12ced97088e7 --- M src/kudu/tablet/cfile_set.cc M src/kudu/tablet/diskrowset-test.cc 2 files changed, 37 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/23/8423/1 -- To view, visit http://gerrit.cloudera.org:8080/8423 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I41b56cbbf1b3b00fdb4dbf1ec08f12ced97088e7 Gerrit-Change-Number: 8423 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 ) Change subject: mvcc: allow tablet shutdown without completing txs .. Patch Set 21: ...flake seemed unrelated... :) -- To view, visit http://gerrit.cloudera.org:8080/7439 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42 Gerrit-Change-Number: 7439 Gerrit-PatchSet: 21 Gerrit-Owner: Andrew WongGerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 31 Oct 2017 02:31:44 + Gerrit-HasComments: No
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Andrew Wong has removed a vote on this change. Change subject: mvcc: allow tablet shutdown without completing txs .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/7439 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42 Gerrit-Change-Number: 7439 Gerrit-PatchSet: 21 Gerrit-Owner: Andrew WongGerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2200: provide better diagnostics when connecting to a subset of masters
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8393 ) Change subject: KUDU-2200: provide better diagnostics when connecting to a subset of masters .. Patch Set 2: Code-Review+1 (3 comments) http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java: http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java@108 PS1, Line 108: // One of the connections should have succeeded. : assertEquals(1, successes); > I chatted with Dan about this on Slack last week. His opinion was that, if Yah I'm just being kind of conservative. If the client always hard fails when the number of masters doesn't match, then it may make it difficult to reconfigure and add/drop masters. I admit it's something of a hypothetical concern, but I think this patch is going to get us most of the bang-for-buck as it is. http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc@344 PS1, Line 344: > leaders are also VOTERs. This is a bit of a forward-looking if statement th Woops, I actually looked up the enum values, but I think I confused Role with MemberType. http://gerrit.cloudera.org:8080/#/c/8393/2/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/8393/2/src/kudu/master/master.cc@352 PS2, Line 352: if (!peer.has_last_known_addr()) { Thanks. This same issue had actually tripped me up in the HMS work (getting the master addrs to add to the HMS entry), so I'm glad to have a general solution available. -- To view, visit http://gerrit.cloudera.org:8080/8393 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56 Gerrit-Change-Number: 8393 Gerrit-PatchSet: 2 Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 31 Oct 2017 01:21:56 + Gerrit-HasComments: Yes
[kudu-CR] Add a macro form for ScopedCleanup
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8416 ) Change subject: Add a macro form for ScopedCleanup .. Add a macro form for ScopedCleanup This adds a new macro SCOPED_CLEANUP({ ... }) which is a shorter form of 'auto cleanup = MakeScopedCleanup([&] { ... }) I also changed over a bunch of call sites to use it (those that were easily discovered by a perl regex). We can switch over the remaining ones as we run across them. Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e Reviewed-on: http://gerrit.cloudera.org:8080/8416 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo--- M src/kudu/fs/log_block_manager.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc M src/kudu/integration-tests/security-unknown-tsk-itest.cc M src/kudu/integration-tests/tombstoned_voting-stress-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/security/ca/cert_management.cc M src/kudu/security/init.cc M src/kudu/security/openssl_util.cc M src/kudu/util/env-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/net/net_util.cc M src/kudu/util/scoped_cleanup-test.cc M src/kudu/util/scoped_cleanup.h M src/kudu/util/threadpool-test.cc 16 files changed, 53 insertions(+), 26 deletions(-) Approvals: Kudu Jenkins: Verified Adar Dembo: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/8416 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e Gerrit-Change-Number: 8416 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add a benchmark test for tablet deletion
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8420 ) Change subject: Add a benchmark test for tablet deletion .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf Gerrit-Change-Number: 8420 Gerrit-PatchSet: 3 Gerrit-Owner: Hao HaoGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 31 Oct 2017 00:39:27 + Gerrit-HasComments: No
[kudu-CR] KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8345 ) Change subject: KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3 Gerrit-Change-Number: 8345 Gerrit-PatchSet: 2 Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 31 Oct 2017 00:39:07 + Gerrit-HasComments: No
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/7439 ) Change subject: mvcc: allow tablet shutdown without completing txs .. Patch Set 21: (26 comments) http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc File src/kudu/integration-tests/ts_tablet_manager-itest.cc: http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@77 PS20, Line 77: using itest::SimpleIntKeyKuduSchema; > warning: using decl 'InternalMiniCluster' is unused [misc-unused-using-decl Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@130 PS20, Line 130: ASSERT_OK(cluster->Start()); > this should probably be less specific to implementation details Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@179 PS20, Line 179: FLAGS_enable_leader_failure_detection = false; > I'm not sure this test suite is the best place for these tests, since they I moved to stop_tablet-itest.cc http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@183 PS20, Line 183: FLAGS_allow_unsafe_replication_factor = true; > here you're stopping a random server, not necessarily the one that the scan Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@187 PS20, Line 187: > are the readers using fault-tolerant scans by default here? Yeah they are (see test_workload.cc:219) http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@197 PS20, Line 197: > Somewhere we have a utility function like AssertNoCrashes(). Otherwise mayb Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@205 PS20, Line 205: ValueDeleter deleter(_map); > in these tests it might be beneficial to configure the MM such that it cons Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@235 PS20, Line 235: > is it possible in this test to re-start the tserver that has the stopped ta I think it'd be a bit tricky in this patch since this makes use of IMC instead of EMC, unless there's a ClusterVerifier equivalent for IMCs that I'm not aware of. Good point though, in EMC tests I've been verifying with ClusterVerifier. Trickier still, this actually makes use of the fact that we're using IMC by stopping the tablet directly by reaching into the server, so I'd prefer to keep this test IMC. http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/integration-tests/ts_tablet_manager-itest.cc@238 PS20, Line 238: // The MarkDirty() callback is on an async thread so it might take the > can you combine this test with the above one, using a utility function that Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc File src/kudu/tablet/mvcc-test.cc: http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc@498 PS20, Line 498: waiting_thread.join(); : ASSERT_OK(s); > don't you want to join before assert here? Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc-test.cc@598 PS20, Line 598: waiting_thread.join(); : ASSERT_STR_CONTAINS(s.ToString(), "closed"); : ASSERT_TRUE(s.IsAborted()); : : // New waiters should abort immediately. : s = mgr.WaitForApplyingTransactionsToCommit(); : > if you reordered the join and the ASSERT then you could avoid the complexit Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h File src/kudu/tablet/mvcc.h: http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@253 PS20, Line 253: Status:: > specify the type of status Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@254 PS20, Line 254: Status WaitForApplyingTransactionsToCommit() const WARN_UNUSED_RESULT; > worth adding a WARN_UNUSED_RESULT here Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@275 PS20, Line 275: not > not? Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/mvcc.h@280 PS20, Line 280: e bool i > rename to is_closed since it's inlinable short method Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@160 PS20, Line 160: // > mind adding a WARN_UNUSED_RESULT to these functions that you're changing fr Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@405 PS20, Line 405: > nit: "should" is kinda fuzzy. Do you mean "will"? Nicer for callers to thin Done http://gerrit.cloudera.org:8080/#/c/7439/20/src/kudu/tablet/tablet.h@405 PS20, Line 405: :
[kudu-CR] mvcc: allow tablet shutdown without completing txs
Hello Tidy Bot, Mike Percy, David Ribeiro Alves, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7439 to look at the new patch set (#21). Change subject: mvcc: allow tablet shutdown without completing txs .. mvcc: allow tablet shutdown without completing txs Currently, the only way to stop an Applying transaction is to wait for it to finish and Commit it. This constraint was put in place to guarantee on-disk correctness, but is sometimes too strict. E.g. if the tablet is shutting down, the Apply doesn't need to finish. This patch adds the ability to "stop" a tablet by closing its MVCC manager. Once this happens, new Applies will return and not move to the Commit phase, and any methods waiting for the tablet's Applies to Commit (e.g. new snapshot scans, FlushMRS) will respond with an error immediately. Applies that are already underway may still Commit, but these Committed operations are inconsequential w.r.t. consistency; having some in-flight transactions Commit and others not is consistent with the server crashing in between the Commits of two transactions. Additionally, once the MVCC manager is closed, new transactions will abort immediately before even reaching the Prepare phase. This will be particularly useful in handling disk failures: if a tablet needs to be shut down due to disk failure, its MVCC manager can be closed immediately, allowing currently-Applying transactions to abort without Committing. This patch only includes the behavior when shutting down a tablet, with the assumption that a tablet will only be shut down when it's being deleted and we don't care too much about its in-flight transactions Committing. Code paths that previously crashed Kudu if Applies did not succeed will now not crash if the MVCC manager is closed and log a warning instead. Testing is done by adding the following: - a test in mvcc-test to shut down MVCC and delete an Applying transaction, ensuring that there are no errors when it leaves scope. - a test in mvcc-test to wait on an Applying transaction, shut down MVCC, and ensure that any waiters will return with an error. - a new test stop_tablet-itest is added to ensure stopped leaders don't complete writes and stopped followers do, and stopped tablets don't prevent fault-tolerant scans Change-Id: I983620f27e7226806a2cca253db7619731914d42 --- M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/stop_tablet-itest.cc M src/kudu/tablet/mvcc-test.cc M src/kudu/tablet/mvcc.cc M src/kudu/tablet/mvcc.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_replica.cc M src/kudu/tablet/tablet_replica_mm_ops.cc M src/kudu/tablet/transactions/transaction_driver.cc M src/kudu/tablet/transactions/transaction_driver.h M src/kudu/tablet/transactions/write_transaction.cc M src/kudu/tserver/tablet_service.cc 13 files changed, 505 insertions(+), 75 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/39/7439/21 -- To view, visit http://gerrit.cloudera.org:8080/7439 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I983620f27e7226806a2cca253db7619731914d42 Gerrit-Change-Number: 7439 Gerrit-PatchSet: 21 Gerrit-Owner: Andrew WongGerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add a benchmark test for tablet deletion
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8420 to look at the new patch set (#3). Change subject: Add a benchmark test for tablet deletion .. Add a benchmark test for tablet deletion Commit 15f3f9b2f optimizes the performance of group blocks deletion via coalescing holes punch for log block manager. This patch adds a test to benchmark tablet deletion for measuring actual improvement. With 2.5k blocks on the tablet: Before commit 15f3f9b2f: I1030 15:25:38.700058 145431 tablet_server-test.cc:2336] Time spent deleting tablet: real 0.182s user 0.000s sys 0.000s I1030 15:25:38.700096 145431 tablet_server-test.cc:2345] block_count_before_delete : 2500 I1030 15:25:38.700109 145431 tablet_server-test.cc:2346] log_block_manager_containers : 6 I1030 15:25:38.700124 145431 tablet_server-test.cc:2347] log_block_manager_holes_punched : 3000 After: I1030 15:31:54.720243 24610 tablet_server-test.cc:2336] Time spent deleting tablet: real 0.062s user 0.001s sys 0.000s I1030 15:31:54.720258 24610 tablet_server-test.cc:2345] block_count_before_delete : 2500 I1030 15:31:54.720268 24610 tablet_server-test.cc:2346] log_block_manager_containers : 6 I1030 15:31:54.720280 24610 tablet_server-test.cc:2347] log_block_manager_holes_punched : 638 Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf --- M src/kudu/tserver/tablet_server-test.cc 1 file changed, 53 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/8420/3 -- To view, visit http://gerrit.cloudera.org:8080/8420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf Gerrit-Change-Number: 8420 Gerrit-PatchSet: 3 Gerrit-Owner: Hao HaoGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Add a benchmark test for tablet deletion
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8420 ) Change subject: Add a benchmark test for tablet deletion .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2293 PS2, Line 2293: uint64_t rows = FLAGS_delete_tablet_bench_num_flushes; > Not needed anymore? Done http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2327 PS2, Line 2327: for > Nit: remove this word Done -- To view, visit http://gerrit.cloudera.org:8080/8420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf Gerrit-Change-Number: 8420 Gerrit-PatchSet: 2 Gerrit-Owner: Hao HaoGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 31 Oct 2017 00:34:33 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager
Hello David Ribeiro Alves, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8345 to look at the new patch set (#2). Change subject: KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager .. KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager The TSTabletManager map is sometimes held for a long time. Given that, a sleeping mutex is more appropriate than a spinlock. Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3 --- M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 2 files changed, 25 insertions(+), 24 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/8345/2 -- To view, visit http://gerrit.cloudera.org:8080/8345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3 Gerrit-Change-Number: 8345 Gerrit-PatchSet: 2 Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add a benchmark test for tablet deletion
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8420 ) Change subject: Add a benchmark test for tablet deletion .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2293 PS2, Line 2293: uint64_t rows = FLAGS_delete_tablet_bench_num_flushes; Not needed anymore? http://gerrit.cloudera.org:8080/#/c/8420/2/src/kudu/tserver/tablet_server-test.cc@2327 PS2, Line 2327: for Nit: remove this word -- To view, visit http://gerrit.cloudera.org:8080/8420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf Gerrit-Change-Number: 8420 Gerrit-PatchSet: 2 Gerrit-Owner: Hao HaoGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 31 Oct 2017 00:22:50 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8345 ) Change subject: KUDU-2193 (part 1): switch to a waiting mutex in TSTabletManager .. Patch Set 1: > Moving to an overall better lock implementation should reduce our cognitive > load here quite a bit - a good lock like absl::Mutex should have the same > performance as a spinlock but also be smart enough to go to sleep when > necessary instead of spinning. Adar told me on slack: "@tlipcon I think we can merge it, I don't feel strongly that the `TSTabletManager` lock be a spinlock. Looks like there's an IWYU failure though." So I'll rev this to fix the build issue -- To view, visit http://gerrit.cloudera.org:8080/8345 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I763abddd74d8b1dabb618318dc84256b533077e3 Gerrit-Change-Number: 8345 Gerrit-PatchSet: 1 Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 31 Oct 2017 00:22:13 + Gerrit-HasComments: No
[kudu-CR] KUDU-2200: provide better diagnostics when connecting to a subset of masters
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8393 ) Change subject: KUDU-2200: provide better diagnostics when connecting to a subset of masters .. Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java File java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java: http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java@238 PS1, Line 238: > pull this into a local variable. Done http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java: http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@311 PS1, Line 311: PB pb : pbs) { > simpler as 'new ArrayList<>(pbs.size());' Done http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.h File src/kudu/master/master.h: http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.h@94 PS1, Line 94: // > warning: missing username/bug in TODO [google-readability-todo] Done http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc@335 PS1, Line 335: t > whitespace Done http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.proto@616 PS1, Line 616: // > weirdly I found that in a non-replicated master setup the RaftConfigPB didn looked into this more and it turns out that the RaftConfigPB does list the local peer but doesn't list its hostname/port, which I guess is on purpose so you could easily renumber/rename your master without confusing it. Changed the code here to fill in the current hostname/port for this case. http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.proto@620 PS1, Line 620: nly talk t > 'Client implementations' or 'Clients' Done -- To view, visit http://gerrit.cloudera.org:8080/8393 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56 Gerrit-Change-Number: 8393 Gerrit-PatchSet: 2 Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 31 Oct 2017 00:19:11 + Gerrit-HasComments: Yes
[kudu-CR] Add a benchmark test for tablet deletion
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8420 ) Change subject: Add a benchmark test for tablet deletion .. Patch Set 2: (10 comments) http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@131 PS1, Line 131: DEFINE_int32(delete_tablet_bench_num_flushes, 200, : "Number of disk row sets to flush in the delete tablet benchmark"); : > Nit: the name and description of this new flag suggests that it's part of t Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2293 PS1, Line 2293: uint64_t rows = FLAGS_delete_tablet_bench_num_flushes; > I don't think this is necessary; the other benchmark in this test isn't con Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2295 PS1, Line 2295: // Collect some related metrics. : scoped_refptrblock_count = > How long does this take to run in slow mode? If it's less than 5s, I don't With 'delete_tablet_bench_num_flushes' default to 200, the test ran for 3s. So I will remove it. http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2299 PS1, Line 2299: scoped_refptr container = > Nit: terminate with a period. Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2300 PS1, Line 2300: METRIC_log_block_manager_containers.Instantiate( > Isn't this guaranteed to be true by the test harness itself? Perhaps it can Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2303 PS1, Line 2303: METRIC_log_block_manager_holes_punched.Instantiate( : mini_server_->server()->metric_entity()); : : // Put some data in the tablet. We insert rows and flush immediately to : // ensure that there is enough blocks on disk to run the benchmark. : for (int i = 0; i < rows; i++) { : NO_FATALS(InsertTestRowsRemote(i, 1)); : ASSERT_OK(tablet_replica_->tablet()->Flush()); : } > I presume this is okay to execute even when block_manager=file? Right, it is safe to do so. http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2314 PS1, Line 2314: ablet from within > Nit: replace with "run the" Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2316 PS1, Line 2316: // by the test code. > Nit: new code should use the shorter NO_FATALS() macro. Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2335 PS1, Line 2335: // Log the related metrics. > No latency is being measured here. Done http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2344 PS1, Line 2344: // Verify that the tablet exists > You can log this regardless of block manager, no? Right, will remove the condition. -- To view, visit http://gerrit.cloudera.org:8080/8420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf Gerrit-Change-Number: 8420 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 31 Oct 2017 00:18:35 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2200: provide better diagnostics when connecting to a subset of masters
Hello Tidy Bot, Alexey Serbin, Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8393 to look at the new patch set (#2). Change subject: KUDU-2200: provide better diagnostics when connecting to a subset of masters .. KUDU-2200: provide better diagnostics when connecting to a subset of masters This changes the ConnectToMaster RPC to send back the list of the master peers, and then changes the clients to use this information to provide better error messages in the case that the user has specified only a subset of the live masters. Note that this patch follows a "first do no harm" philosophy. In the case that the user "gets lucky" and only specifies one of their three masters, and that master happens to be the leader, we'll continue to treat that as "successful". Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56 --- M java/kudu-client/src/main/java/org/apache/kudu/client/ConnectToCluster.java M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java M java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java M src/kudu/client/master_rpc.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/master/master-test.cc M src/kudu/master/master.cc M src/kudu/master/master.h M src/kudu/master/master.proto M src/kudu/master/master_service.cc 10 files changed, 254 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/8393/2 -- To view, visit http://gerrit.cloudera.org:8080/8393 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56 Gerrit-Change-Number: 8393 Gerrit-PatchSet: 2 Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add a benchmark test for tablet deletion
Hello Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8420 to look at the new patch set (#2). Change subject: Add a benchmark test for tablet deletion .. Add a benchmark test for tablet deletion Commit 15f3f9b2f optimizes the performance of group blocks deletion via coalescing holes punch for log block manager. This patch adds a test to benchmark tablet deletion for measuring actual improvement. With 2.5k blocks on the tablet: Before commit 15f3f9b2f: I1030 15:25:38.700058 145431 tablet_server-test.cc:2336] Time spent deleting tablet: real 0.182s user 0.000s sys 0.000s I1030 15:25:38.700096 145431 tablet_server-test.cc:2345] block_count_before_delete : 2500 I1030 15:25:38.700109 145431 tablet_server-test.cc:2346] log_block_manager_containers : 6 I1030 15:25:38.700124 145431 tablet_server-test.cc:2347] log_block_manager_holes_punched : 3000 After: I1030 15:31:54.720243 24610 tablet_server-test.cc:2336] Time spent deleting tablet: real 0.062s user 0.001s sys 0.000s I1030 15:31:54.720258 24610 tablet_server-test.cc:2345] block_count_before_delete : 2500 I1030 15:31:54.720268 24610 tablet_server-test.cc:2346] log_block_manager_containers : 6 I1030 15:31:54.720280 24610 tablet_server-test.cc:2347] log_block_manager_holes_punched : 638 Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf --- M src/kudu/tserver/tablet_server-test.cc 1 file changed, 55 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/8420/2 -- To view, visit http://gerrit.cloudera.org:8080/8420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf Gerrit-Change-Number: 8420 Gerrit-PatchSet: 2 Gerrit-Owner: Hao HaoGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [mini-cluster] allow 18-bit PID in LOOPBACK bind mode
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8422 ) Change subject: [mini-cluster] allow 18-bit PID in LOOPBACK bind mode .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/8422/1/src/kudu/mini-cluster/mini_cluster.cc File src/kudu/mini-cluster/mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/8422/1/src/kudu/mini-cluster/mini_cluster.cc@37 PS1, Line 37: static const uint8_t INDEX_MAX = 0x3f; // 2^6 - 1 I think this function could probably be rewritten to be a bit clearer now that it's more complicated than it was before. The magic numbers here make it hard to follow. something along the lines of: int kPidBits = 18; int kServerBits = 24 - kPidBits; CHECK_LT(index, 1 << kServerBits); CHECK_LT(pid, 1 << kPidBits); uint32_t ip = (pid << kServerBits) | kServer; ... extract octets from 'ip' -- To view, visit http://gerrit.cloudera.org:8080/8422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130 Gerrit-Change-Number: 8422 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 31 Oct 2017 00:13:33 + Gerrit-HasComments: Yes
[kudu-CR] [mini-cluster] allow 18-bit PID in LOOPBACK bind mode
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8422 Change subject: [mini-cluster] allow 18-bit PID in LOOPBACK bind mode .. [mini-cluster] allow 18-bit PID in LOOPBACK bind mode This changelist allows to use 18-bit PID for binding in LOOPBACK mode. Prior to this patch, the minicluster supported PIDs to be up to 16 bits wide. While expanding the range for PIDs, this also narrows down the limit on maximum number of 'indexed daemons' to 62 (it was 262 prior to this change). Hopefully, that's enough for our minicluster tests. The incentive to put up this patch was that I recently found one of our build servers ending up with 147456 as pid_max (/proc/sys/kernel/pid_max) after recent restart. Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130 --- M src/kudu/mini-cluster/mini_cluster.cc 1 file changed, 12 insertions(+), 4 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/22/8422/1 -- To view, visit http://gerrit.cloudera.org:8080/8422 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I958729d71ed363e98bea99f3d932cc6b0546e130 Gerrit-Change-Number: 8422 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin
[kudu-CR] Add a benchmark test for tablet deletion
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8420 ) Change subject: Add a benchmark test for tablet deletion .. Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@131 PS1, Line 131: DEFINE_int32(single_threaded_delete_tablet_bench_insert_rows, 500, : "Number of rows to insert in the testing phase of the single threaded " : "tablet server delete tablet benchmark"); Nit: the name and description of this new flag suggests that it's part of the existing "insert latency micro-benchmark". Perhaps call it "delete_tablet_bench_num_flushes" and describe it with "Number of disk row sets to flush in the delete tablet benchmark"? http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2293 PS1, Line 2293: #ifdef NDEBUG I don't think this is necessary; the other benchmark in this test isn't conditioned on RELEASE mode. http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2295 PS1, Line 2295: uint64_t rows = AllowSlowTests() ? : FLAGS_single_threaded_delete_tablet_bench_insert_rows : 10; How long does this take to run in slow mode? If it's less than 5s, I don't think it's worth the condition. http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2299 PS1, Line 2299: // Verify that the tablet exists Nit: terminate with a period. http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2300 PS1, Line 2300: ASSERT_TRUE(mini_server_->server()->tablet_manager()->LookupTablet(kTabletId, )); Isn't this guaranteed to be true by the test harness itself? Perhaps it can be omitted in favor of operating on tablet_replica_ directly? http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2303 PS1, Line 2303: scoped_refptrblock_count = : METRIC_log_block_manager_blocks_under_management.Instantiate( : mini_server_->server()->metric_entity(), 0); : scoped_refptr container = : METRIC_log_block_manager_containers.Instantiate( : mini_server_->server()->metric_entity(), 0); : scoped_refptr holes_punched = : METRIC_log_block_manager_holes_punched.Instantiate( : mini_server_->server()->metric_entity()); I presume this is okay to execute even when block_manager=file? http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2314 PS1, Line 2314: do tablet deletion Nit: replace with "run the" http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2316 PS1, Line 2316: ASSERT_NO_FATAL_FAILURE(InsertTestRowsRemote(i, 1)); Nit: new code should use the shorter NO_FATALS() macro. http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2335 PS1, Line 2335: // Send the call and measure the latency. No latency is being measured here. http://gerrit.cloudera.org:8080/#/c/8420/1/src/kudu/tserver/tablet_server-test.cc@2344 PS1, Line 2344: if (FLAGS_block_manager == "log") { You can log this regardless of block manager, no? -- To view, visit http://gerrit.cloudera.org:8080/8420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf Gerrit-Change-Number: 8420 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 30 Oct 2017 23:37:00 + Gerrit-HasComments: Yes
[kudu-CR] [java client] improve AsyncKuduScanner logging
Hello Alexey Serbin, Dan Burkert, Jean-Daniel Cryans, Kudu Jenkins, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8382 to look at the new patch set (#4). Change subject: [java client] improve AsyncKuduScanner logging .. [java client] improve AsyncKuduScanner logging As filed in KUDU-2199, in a jenkin's job build, ITClient test failed even with fault tolerant scanner. The test log demonstrates that the issue may cause by re-scan of scanned data. However, loop ITClient 1 times and did not discover any issues: http://dist-test.cloudera.org/job?job_id=hao.hao.1508869382.32617 This patch improves test coverage of ITClient so that error will be reported if row count of each scan ever unexpectedly decrease. It also improves logging of AsyncKuduScanner to give more information for diagnosis if the same issue occur again. Another 2000 runs of ITClient with current patch passed: http://dist-test.cloudera.org/job?job_id=hao.hao.1509045968.14317 Change-Id: I90ffcd01e7f99f3090fa118092fc303e06fb92dc --- M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java R java/kudu-client/src/main/java/org/apache/kudu/client/FaultTolerantScannerExpiredException.java M java/kudu-client/src/test/java/org/apache/kudu/client/ITClient.java 3 files changed, 22 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/82/8382/4 -- To view, visit http://gerrit.cloudera.org:8080/8382 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I90ffcd01e7f99f3090fa118092fc303e06fb92dc Gerrit-Change-Number: 8382 Gerrit-PatchSet: 4 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [java client] improve AsyncKuduScanner logging
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8382 ) Change subject: [java client] improve AsyncKuduScanner logging .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/8382/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/8382/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@727 PS3, Line 727: ret += ", lastPrimaryKey = " + Bytes.pretty(lastPrimaryKey); > I'd lean on the side of not exposing it, since it's somewhat complicated to Ok, then will remove it. As on the other hand, hashed lastPrimaryKey can only give very limited information. -- To view, visit http://gerrit.cloudera.org:8080/8382 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I90ffcd01e7f99f3090fa118092fc303e06fb92dc Gerrit-Change-Number: 8382 Gerrit-PatchSet: 3 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 23:24:12 + Gerrit-HasComments: Yes
[kudu-CR] Add a benchmark test for tablet deletion
Hao Hao has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8420 Change subject: Add a benchmark test for tablet deletion .. Add a benchmark test for tablet deletion Commit 15f3f9b2f optimizes the performance of group blocks deletion via coalescing holes punch for log block manager. This patch adds a test to benchmark tablet deletion for measuring actual improvement. With 2.5k blocks on the tablet: Before commit 15f3f9b2f: I1030 15:25:38.700058 145431 tablet_server-test.cc:2336] Time spent deleting tablet: real 0.182s user 0.000s sys 0.000s I1030 15:25:38.700096 145431 tablet_server-test.cc:2345] block_count_before_delete : 2500 I1030 15:25:38.700109 145431 tablet_server-test.cc:2346] log_block_manager_containers : 6 I1030 15:25:38.700124 145431 tablet_server-test.cc:2347] log_block_manager_holes_punched : 3000 After: I1030 15:31:54.720243 24610 tablet_server-test.cc:2336] Time spent deleting tablet: real 0.062s user 0.001s sys 0.000s I1030 15:31:54.720258 24610 tablet_server-test.cc:2345] block_count_before_delete : 2500 I1030 15:31:54.720268 24610 tablet_server-test.cc:2346] log_block_manager_containers : 6 I1030 15:31:54.720280 24610 tablet_server-test.cc:2347] log_block_manager_holes_punched : 638 Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf --- M src/kudu/tserver/tablet_server-test.cc 1 file changed, 66 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/8420/1 -- To view, visit http://gerrit.cloudera.org:8080/8420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I6054b55f81e5a6e6febcbb5781ad8d776c49a5cf Gerrit-Change-Number: 8420 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao
[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8304 ) Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster .. Patch Set 9: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc File src/kudu/mini-cluster/external_mini_cluster-test.cc: http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc@28 PS9, Line 28: #include "kudu/gutil/strings/util.h" // IWYU pragma: keep > HasPrefixString And IWYU wants you to remove it due to the other changes in the file? Weird. -- To view, visit http://gerrit.cloudera.org:8080/8304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 Gerrit-Change-Number: 8304 Gerrit-PatchSet: 9 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 23:08:12 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 ) Change subject: KUDU-2191 (2/n): Hive Metastore client .. Patch Set 29: (1 comment) http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h File src/kudu/hms/mini_hms.h: http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/mini_hms.h@30 PS29, Line 30: #include "kudu/util/subprocess.h" > You sure about that? I'm looking at https://stackoverflow.com/questions/601 Sorry for not responding before. This appears to me to be toolchain dependent (probably libstdcxx vs libcxx), because this won't compile on macos (libcxx) with a forwards declare. I'm not crazy about the idea of forgoing the simplicity of '= default;' just to satisfy IWYU, so I'd like to keep it as is. -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 29 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 23:06:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8304 ) Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc File src/kudu/mini-cluster/external_mini_cluster-test.cc: http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc@28 PS9, Line 28: #include "kudu/gutil/strings/util.h" // IWYU pragma: keep > What's util.h used for? HasPrefixString -- To view, visit http://gerrit.cloudera.org:8080/8304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 Gerrit-Change-Number: 8304 Gerrit-PatchSet: 9 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 22:41:57 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 ) Change subject: KUDU-2191 (2/n): Hive Metastore client .. Patch Set 36: (1 comment) http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h File src/kudu/hms/mini_hms.h: http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h@30 PS36, Line 30: #include "kudu/util/subprocess.h" // IWYU pragma: keep > Any particular reason you don't want to move MiniHms constructor into mini_ I don't like the idea of making the code more complex in order to satisfy IWYU. -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 36 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 22:41:15 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8304 ) Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster .. Patch Set 9: (1 comment) http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc File src/kudu/mini-cluster/external_mini_cluster-test.cc: http://gerrit.cloudera.org:8080/#/c/8304/9/src/kudu/mini-cluster/external_mini_cluster-test.cc@28 PS9, Line 28: #include "kudu/gutil/strings/util.h" // IWYU pragma: keep What's util.h used for? -- To view, visit http://gerrit.cloudera.org:8080/8304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 Gerrit-Change-Number: 8304 Gerrit-PatchSet: 9 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 22:15:10 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 ) Change subject: KUDU-2191 (2/n): Hive Metastore client .. Patch Set 36: (1 comment) http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h File src/kudu/hms/mini_hms.h: http://gerrit.cloudera.org:8080/#/c/7053/36/src/kudu/hms/mini_hms.h@30 PS36, Line 30: #include "kudu/util/subprocess.h" // IWYU pragma: keep Any particular reason you don't want to move MiniHms constructor into mini_hms.cc, thus allowing you to cleanly forward declare Subprocess and avoid this inclusion? Alexey and I both suggested that back in PS29/PS30. -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 36 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 22:12:48 + Gerrit-HasComments: Yes
[kudu-CR] [webui] Add templates for tserver webui
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8307 ) Change subject: [webui] Add templates for tserver webui .. Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.h File src/kudu/tserver/tserver_path_handlers.h: http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.h@27 PS7, Line 27: template class scoped_refptr; > warning: invalid case style for class 'scoped_refptr' [readability-identifi :( http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc File src/kudu/tserver/tserver_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc@170 PS7, Line 170: transaction_json["desc"] = std::move(inflight_tx.description()); > warning: std::move of the const expression has no effect; remove std::move( Done http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc@172 PS7, Line 172: transaction_json["trace"] = std::move(inflight_tx.trace_buffer()); > warning: std::move of the const expression has no effect; remove std::move( Done http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc@214 PS7, Line 214: maptablet_statuses; > nit: maybe rename this to tablet_states? Done http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc@215 PS7, Line 215: for (const scoped_refptr& replica : replicas) { > Can this loop be merged with the other loop over `replicas`? Seems like the Done http://gerrit.cloudera.org:8080/#/c/8307/7/src/kudu/tserver/tserver_path_handlers.cc@240 PS7, Line 240: tablet_detail_json["target"] = Substitute("/tablet?id=$0", status.tablet_id()); > Does the /tablet page exist if replica exists but the tablet doesn't exist? Technically the /tablet page does exist if the replica exists but the tablet doesn't, but there's not going to be anything useful about it anymore. -- To view, visit http://gerrit.cloudera.org:8080/8307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I99e08be9aa8abddd51ada61683b6f75190a00b5c Gerrit-Change-Number: 8307 Gerrit-PatchSet: 7 Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 30 Oct 2017 22:10:54 + Gerrit-HasComments: Yes
[kudu-CR] [webui] Add templates for tserver webui
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8307 to look at the new patch set (#8). Change subject: [webui] Add templates for tserver webui .. [webui] Add templates for tserver webui This converts all the tablet server web ui endpoints to use a template, unless it doesn't make sense to. This includes - /scans - /tablets - /tablet - /transactions - /tablet-rowsetlayout-svg - /tablet-consensus-status - /log-anchors - /dashboards Change-Id: I99e08be9aa8abddd51ada61683b6f75190a00b5c --- M src/kudu/server/webserver.cc M src/kudu/server/webui_util.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tserver_path_handlers.cc M src/kudu/tserver/tserver_path_handlers.h A www/dashboards.mustache A www/log-anchors.mustache A www/scans.mustache M www/table.mustache A www/tablet-consensus-status.mustache A www/tablet-rowsetlayout-svg.mustache A www/tablet.mustache A www/tablets.mustache A www/transactions.mustache 14 files changed, 604 insertions(+), 384 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/8307/8 -- To view, visit http://gerrit.cloudera.org:8080/8307 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I99e08be9aa8abddd51ada61683b6f75190a00b5c Gerrit-Change-Number: 8307 Gerrit-PatchSet: 8 Gerrit-Owner: Will BerkeleyGerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] WIP: KUDU-2200: provide better diagnostics when connecting to a subset of masters
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8393 ) Change subject: WIP: KUDU-2200: provide better diagnostics when connecting to a subset of masters .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/8393/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8393/1//COMMIT_MSG@9 PS1, Line 9: This changes the ConnectToMaster RPC to send back the list of the master : peers, and then changes the client to use this information to provide : better error messages in the case that the user has specified only a : subset of the live masters. > How does this fit into the case of migration from single to multi-master se The CLI tool during migration is using raw RPCs against the masters rather than the client, so I think it should be fine. http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java: http://gerrit.cloudera.org:8080/#/c/8393/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestConnectToCluster.java@108 PS1, Line 108: // One of the connections should have succeeded. : assertEquals(1, successes); > I don't understand this. I would expect all connections to fail because it I chatted with Dan about this on Slack last week. His opinion was that, if the user happens to "guess right" and gets the leader master, we shouldn't proactively fail them. I think his thinking was "first do no harm" -- ie not cause any breakage in a situation that would otherwise have worked. Dan, mind piping up with your reasoning here? http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.cc@344 PS1, Line 344: if (peer.member_type() == consensus::RaftPeerPB::VOTER) { > Is filtering leader masters intentional? leaders are also VOTERs. This is a bit of a forward-looking if statement that anticipates the possibility of non-voter leaders http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/8393/1/src/kudu/master/master.proto@616 PS1, Line 616: // NOTE: This will not be filled in on a single-master cluster. > +1 weirdly I found that in a non-replicated master setup the RaftConfigPB didn't have any masters. But I'll look again, maybe I did something wrong when I was trying this. -- To view, visit http://gerrit.cloudera.org:8080/8393 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I52f903e1aa5ae6948ca1ba6d4d856c3c9dc73d56 Gerrit-Change-Number: 8393 Gerrit-PatchSet: 1 Gerrit-Owner: Todd LipconGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 21:41:20 + Gerrit-HasComments: Yes
[kudu-CR] Add a macro form for ScopedCleanup
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8416 ) Change subject: Add a macro form for ScopedCleanup .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/8416/1/src/kudu/util/scoped_cleanup.h File src/kudu/util/scoped_cleanup.h: http://gerrit.cloudera.org:8080/#/c/8416/1/src/kudu/util/scoped_cleanup.h@31 PS1, Line 31: // NOTE: in the case that you want to cancel the cleanup, use the more verbose : // (non-macro) form below. > Worth mentioning that this form automatically captures everything by ref? eh, given that the definition is only a single line and right here next to the comment, I think it's a bit redundant. -- To view, visit http://gerrit.cloudera.org:8080/8416 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e Gerrit-Change-Number: 8416 Gerrit-PatchSet: 1 Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 21:28:51 + Gerrit-HasComments: Yes
[kudu-CR] Add a macro form for ScopedCleanup
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8416 ) Change subject: Add a macro form for ScopedCleanup .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/8416/1/src/kudu/util/scoped_cleanup.h File src/kudu/util/scoped_cleanup.h: http://gerrit.cloudera.org:8080/#/c/8416/1/src/kudu/util/scoped_cleanup.h@31 PS1, Line 31: // NOTE: in the case that you want to cancel the cleanup, use the more verbose : // (non-macro) form below. Worth mentioning that this form automatically captures everything by ref? -- To view, visit http://gerrit.cloudera.org:8080/8416 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e Gerrit-Change-Number: 8416 Gerrit-PatchSet: 1 Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 30 Oct 2017 21:13:25 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2205. Improve error message for failed globs
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8415 ) Change subject: KUDU-2205. Improve error message for failed globs .. KUDU-2205. Improve error message for failed globs This fixes the Env::Glob function to give a more understandable error message on typical issues such as permission denied. This can improve the diagnosability in the case that a server is started with an inaccessible log directory. Change-Id: Ib7e487b5f6b24c2a2bd66e33f5b51a31e6585657 Reviewed-on: http://gerrit.cloudera.org:8080/8415 Reviewed-by: Adar DemboTested-by: Kudu Jenkins --- M src/kudu/util/env-test.cc M src/kudu/util/env_posix.cc 2 files changed, 21 insertions(+), 1 deletion(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/8415 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ib7e487b5f6b24c2a2bd66e33f5b51a31e6585657 Gerrit-Change-Number: 8415 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley
[kudu-CR] Add a macro form for ScopedCleanup
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8416 to review the following change. Change subject: Add a macro form for ScopedCleanup .. Add a macro form for ScopedCleanup This adds a new macro SCOPED_CLEANUP({ ... }) which is a shorter form of 'auto cleanup = MakeScopedCleanup([&] { ... }) I also changed over a bunch of call sites to use it (those that were easily discovered by a perl regex). We can switch over the remaining ones as we run across them. Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e --- M src/kudu/fs/log_block_manager.cc M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/raft_consensus_nonvoter-itest.cc M src/kudu/integration-tests/security-unknown-tsk-itest.cc M src/kudu/integration-tests/tombstoned_voting-stress-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/security/ca/cert_management.cc M src/kudu/security/init.cc M src/kudu/security/openssl_util.cc M src/kudu/util/env-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/net/net_util.cc M src/kudu/util/scoped_cleanup-test.cc M src/kudu/util/scoped_cleanup.h M src/kudu/util/threadpool-test.cc 16 files changed, 53 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/8416/1 -- To view, visit http://gerrit.cloudera.org:8080/8416 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Idb72c27824def3c6a6d98c56b46cdd620f651e0e Gerrit-Change-Number: 8416 Gerrit-PatchSet: 1 Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo
[kudu-CR] KUDU-2205. Improve error message for failed globs
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8415 ) Change subject: KUDU-2205. Improve error message for failed globs .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8415 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib7e487b5f6b24c2a2bd66e33f5b51a31e6585657 Gerrit-Change-Number: 8415 Gerrit-PatchSet: 1 Gerrit-Owner: Todd LipconGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley Gerrit-Comment-Date: Mon, 30 Oct 2017 19:36:10 + Gerrit-HasComments: No
[kudu-CR] [build] fix OPENSSL ROOT DIR override on RH/CentOS
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8407 ) Change subject: [build] fix OPENSSL_ROOT_DIR override on RH/CentOS .. [build] fix OPENSSL_ROOT_DIR override on RH/CentOS Due to an extra level of indirection in the condition for one if() expression, the OpenSSL location was not overridden on RedHat/CentOS machines where the OpenSSL workaround build existed in $KUDU_ROOT/thirdparty/installed/openssl-el6-workaround, even if the OPENSSL_ROOT_DIR was overridden for cmake invocation (i.e. -DOPENSSL_ROOT_DIR=/path/to/alt-openssl-location was specified). Removing the level of indirection for the OPENSSL_ROOT_DIR variable in the expression fixed the issue, which seems to be in sync with https://cmake.org/cmake/help/v3.9/command/if.html At least, with this patch the issue is gone if running cmake 3.9.0. Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff Reviewed-on: http://gerrit.cloudera.org:8080/8407 Tested-by: Alexey SerbinReviewed-by: Todd Lipcon --- M CMakeLists.txt 1 file changed, 1 insertion(+), 2 deletions(-) Approvals: Alexey Serbin: Verified Todd Lipcon: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/8407 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff Gerrit-Change-Number: 8407 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2205. Improve error message for failed globs
Hello Will Berkeley, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8415 to review the following change. Change subject: KUDU-2205. Improve error message for failed globs .. KUDU-2205. Improve error message for failed globs This fixes the Env::Glob function to give a more understandable error message on typical issues such as permission denied. This can improve the diagnosability in the case that a server is started with an inaccessible log directory. Change-Id: Ib7e487b5f6b24c2a2bd66e33f5b51a31e6585657 --- M src/kudu/util/env-test.cc M src/kudu/util/env_posix.cc 2 files changed, 21 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/15/8415/1 -- To view, visit http://gerrit.cloudera.org:8080/8415 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ib7e487b5f6b24c2a2bd66e33f5b51a31e6585657 Gerrit-Change-Number: 8415 Gerrit-PatchSet: 1 Gerrit-Owner: Todd LipconGerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8304 ) Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster .. Patch Set 9: This is ready for review again. Sorry for the churn. -- To view, visit http://gerrit.cloudera.org:8080/8304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 Gerrit-Change-Number: 8304 Gerrit-PatchSet: 9 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 19:01:29 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: support parallel multi-master clusters
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8280 ) Change subject: mini-cluster: support parallel multi-master clusters .. Patch Set 2: It's coming back to me now; at least one of the issues is that we rely on lsof to tell us if ports are bound / a service is ready, and the reuse_port trick messes with that. -- To view, visit http://gerrit.cloudera.org:8080/8280 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 2 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 30 Oct 2017 18:55:03 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: support parallel multi-master clusters
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8280 ) Change subject: mini-cluster: support parallel multi-master clusters .. Patch Set 2: I got some time to bang on this on a recent flight, but I still don't think it's in a comittable state. I recall running into an issue with reuse port, but I don't remember what (I recall thinking I'd need a linux test box, and had no internet access). Anyway, I'll pick this back up when I have more time -- To view, visit http://gerrit.cloudera.org:8080/8280 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 2 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Comment-Date: Mon, 30 Oct 2017 18:47:52 + Gerrit-HasComments: No
[kudu-CR] mini-cluster: support parallel multi-master clusters
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8280 to look at the new patch set (#2). Change subject: mini-cluster: support parallel multi-master clusters .. mini-cluster: support parallel multi-master clusters This commit refactors the mini-clusters to internally use reserved sockets for their child daemons. Reserved sockets are simply sockets bound to a random port with SO_REUSEPORT. This allows for a few simplifications: * master addresses in multi-master clusters can be reserved up-front, which removes any chance of a race condition in binding. As a result, multi-master clusters no longer need hard-coded ports. As a result, tests using multi-master clusters no longer need to run in sequence. * external mini cluster tablet servers also now use a reserved port, so that when the tablet server processes is restarted there is no chance of a port conflict. As a result, we no longer are required to use the 'unique loopback' hack on Linux. Internal mini cluster tablet servers still bind to a non-reserved port, since we never restart internal mini-cluster tablet servers. Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c --- M src/kudu/client/client-test.cc M src/kudu/integration-tests/CMakeLists.txt M src/kudu/integration-tests/catalog_manager_tsk-itest.cc M src/kudu/integration-tests/client-negotiation-failover-itest.cc M src/kudu/integration-tests/client-stress-test.cc M src/kudu/integration-tests/master-stress-test.cc M src/kudu/integration-tests/master_cert_authority-itest.cc M src/kudu/integration-tests/master_failover-itest.cc M src/kudu/integration-tests/master_migration-itest.cc M src/kudu/integration-tests/master_replication-itest.cc M src/kudu/integration-tests/security-faults-itest.cc M src/kudu/integration-tests/token_signer-itest.cc M src/kudu/integration-tests/webserver-stress-itest.cc M src/kudu/master/mini_master.cc M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/mini-cluster/internal_mini_cluster.cc M src/kudu/mini-cluster/internal_mini_cluster.h M src/kudu/mini-cluster/mini_cluster.cc M src/kudu/mini-cluster/mini_cluster.h M src/kudu/tools/CMakeLists.txt M src/kudu/tools/ksck_remote-test.cc M src/kudu/tools/tool_action_test.cc 25 files changed, 162 insertions(+), 236 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/8280/2 -- To view, visit http://gerrit.cloudera.org:8080/8280 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0b0ff7bfc179d8fdb1ed306d1bbd12acddeb060c Gerrit-Change-Number: 8280 Gerrit-PatchSet: 2 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] [build] fix OPENSSL ROOT DIR override on RH/CentOS
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8407 ) Change subject: [build] fix OPENSSL_ROOT_DIR override on RH/CentOS .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8407 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe69b5d62ef4054e60138a154a85569d92166aff Gerrit-Change-Number: 8407 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey SerbinGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 18:45:24 + Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 ) Change subject: KUDU-2191 (2/n): Hive Metastore client .. Patch Set 36: OK I think this is ready to review again. Sorry for the churn. -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 36 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 18:40:18 + Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8304 to look at the new patch set (#9). Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster .. KUDU-2191 (3/n): Add mini HMS option to external mini cluster A couple of features are explicitly being punted on for now: - If kerberos and the HMS are both configured, the HMS should be kerberized as well. This will come in a follow-up commit (the current HMS C++ client can't be used against a kerberized HMS). - No API is provided in the mini-cluster tool to retrieve the HMS address. This will be added if/when it becomes necessary. Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 --- M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_test.cc 6 files changed, 79 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8304/9 -- To view, visit http://gerrit.cloudera.org:8080/8304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 Gerrit-Change-Number: 8304 Gerrit-PatchSet: 9 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8304 to look at the new patch set (#8). Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster .. KUDU-2191 (3/n): Add mini HMS option to external mini cluster A couple of features are explicitly being punted on for now: - If kerberos and the HMS are both configured, the HMS should be kerberized as well. This will come in a follow-up commit (the current HMS C++ client can't be used against a kerberized HMS). - No API is provided in the mini-cluster tool to retrieve the HMS address. This will be added if/when it becomes necessary. Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 --- M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_test.cc 6 files changed, 74 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8304/8 -- To view, visit http://gerrit.cloudera.org:8080/8304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 Gerrit-Change-Number: 8304 Gerrit-PatchSet: 8 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Update auth token validity seconds description
Hello Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8413 to review the following change. Change subject: Update auth_token_validity_seconds description .. Update auth_token_validity_seconds description The caveats are no longer relevant since KUDU-2013 landed. Change-Id: I050e39d0377049cdaac0afb5085a8a9a19d620a5 --- M src/kudu/master/master.cc 1 file changed, 2 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/13/8413/1 -- To view, visit http://gerrit.cloudera.org:8080/8413 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I050e39d0377049cdaac0afb5085a8a9a19d620a5 Gerrit-Change-Number: 8413 Gerrit-PatchSet: 1 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Alexey Serbin
[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8304 ) Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster .. Patch Set 6: -Verified (1 comment) http://gerrit.cloudera.org:8080/#/c/8304/6/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/8304/6/src/kudu/mini-cluster/external_mini_cluster.h@138 PS6, Line 138: // If true, set up a Hive Metastore as part of this ExternalMiniCluster. > include a line Default: false/true Done -- To view, visit http://gerrit.cloudera.org:8080/8304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 Gerrit-Change-Number: 8304 Gerrit-PatchSet: 6 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 17:37:50 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (3/n): Add mini HMS option to external mini cluster
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8304 to look at the new patch set (#7). Change subject: KUDU-2191 (3/n): Add mini HMS option to external mini cluster .. KUDU-2191 (3/n): Add mini HMS option to external mini cluster A couple of features are explicitly being punted on for now: - If kerberos and the HMS are both configured, the HMS should be kerberized as well. This will come in a follow-up commit (the current HMS C++ client can't be used against a kerberized HMS). - No API is provided in the mini-cluster tool to retrieve the HMS address. This will be added if/when it becomes necessary. Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 --- M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/mini-cluster/external_mini_cluster-test.cc M src/kudu/mini-cluster/external_mini_cluster.cc M src/kudu/mini-cluster/external_mini_cluster.h M src/kudu/tools/tool.proto M src/kudu/tools/tool_action_test.cc 6 files changed, 73 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/04/8304/7 -- To view, visit http://gerrit.cloudera.org:8080/8304 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iaa1da05b13a120f2d4b739b6ab709e6315c955c0 Gerrit-Change-Number: 8304 Gerrit-PatchSet: 7 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 ) Change subject: KUDU-2191 (2/n): Hive Metastore client .. Patch Set 30: (1 comment) http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py File build-support/run_dist_test.py: http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py@128 PS30, Line 128: env['HIVE_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/apache-hive-*-bin"))[0] : env['HADOOP_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/hadoop-*"))[0] > Done Woops, turns out this was correct the first time. dist_test.py ensures that only the correct versions of hive and hadoop are copied, because they use the build/latest/bin/[hadoop-home|hive-home] symlinks. -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 30 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 16:15:36 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7053 to look at the new patch set (#36). Change subject: KUDU-2191 (2/n): Hive Metastore client .. KUDU-2191 (2/n): Hive Metastore client This patch lays the groundwork for integrating the Kudu catalog with the Hive MetaStore. The focus of this patch is a Kudu-specific C++ HMS client (hms_client.[h|cc]) in a new hms module. This client provides bindings for the Hive Metastore APIs that Kudu will use in follow-up commits. - Thrift has been added as a dependency, and a mechanism for performing Thrift codegen at compile time has been added (see FindThrift.cmake, based on FindProtobuf.cmake) - Bison has been added as a build-time dependency, because the system bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10. - Hive and Hadoop have been added to thirdparty as test-only dependencies. - A Hive MetaStore external mini server is included for testing. See mini_hms.[h|cc]. - The Kudu Metastore plugin is compiled from CMake as a standalone jar for loading into the HMS mini server. Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee --- M CMakeLists.txt M build-support/dist_test.py M build-support/iwyu/iwyu-filter.awk M build-support/run_dist_test.py A cmake_modules/FindJavaHome.cmake A cmake_modules/FindThrift.cmake A src/kudu/hms/CMakeLists.txt A src/kudu/hms/hive_metastore.thrift A src/kudu/hms/hms_client-test.cc A src/kudu/hms/hms_client.cc A src/kudu/hms/hms_client.h A src/kudu/hms/mini_hms.cc A src/kudu/hms/mini_hms.h M thirdparty/LICENSE.txt M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh M thirdparty/download-thirdparty.sh M thirdparty/vars.sh 18 files changed, 2,928 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/36 -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 36 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [java client] improve AsyncKuduScanner logging
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8382 ) Change subject: [java client] improve AsyncKuduScanner logging .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/8382/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java: http://gerrit.cloudera.org:8080/#/c/8382/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduScanner.java@727 PS3, Line 727: ret += ", lastPrimaryKey = " + Bytes.pretty(lastPrimaryKey); > Hmm, you are right. Do you think it makes sense to log a hashed value for d I'd lean on the side of not exposing it, since it's somewhat complicated to use a crypto-secure hash (calling hashCode() isn't sufficient). But if you think it's worth the effort in terms of increased debug-ability, then go for it. -- To view, visit http://gerrit.cloudera.org:8080/8382 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I90ffcd01e7f99f3090fa118092fc303e06fb92dc Gerrit-Change-Number: 8382 Gerrit-PatchSet: 3 Gerrit-Owner: Hao HaoGerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 16:07:16 + Gerrit-HasComments: Yes
[kudu-CR] python: upgrade pip when building client and restore old workarounds
Jean-Daniel Cryans has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8406 ) Change subject: python: upgrade pip when building client and restore old workarounds .. python: upgrade pip when building client and restore old workarounds It appears that pypi recently deactivated their http:// endpoint, which is the default in the older versions of pip found on el6. As a result, pip installation requests fail: Downloading/unpacking pytest (from -r requirements.txt (line 1)) Cannot fetch index base URL http://pypi.python.org/simple/ Could not find any downloads that satisfy the requirement pytest (from -r requirements.txt (line 1)) No distributions at all found for pytest (from -r requirements.txt (line 1)) Passing -i with the https:// endpoint works for the dependencies being installed, but it doesn't appear to be passed down correctly to transitive dependencies, and they fail to install with an inscrutable error [1]. So instead, let's go back to the old approach where we first upgrade pip. This means effectively reverting [2] and restoring [3], but such is life. I tested this by running build-and-test.sh on an el6 machine that created virtualenvs containing pip 1.1 and setuptools 0.6c11. 1. https://stackoverflow.com/questions/5178535/easy-install-libmysqld-dev-error-nonetype-object-has-no-attribute-clone 2. https://github.com/apache/kudu/commit/b198ed8ff6a603816892c8bc7fd80679a014aaf1 3. https://github.com/apache/kudu/commit/d0acb55510c0552605953e07740f46d6be66c9c1 Change-Id: Ia75b65d66a1559ddaefb53dc1e67837faf9e7e6a Reviewed-on: http://gerrit.cloudera.org:8080/8406 Tested-by: Kudu Jenkins Reviewed-by: Jean-Daniel Cryans--- M build-support/jenkins/build-and-test.sh M python/requirements.txt 2 files changed, 42 insertions(+), 3 deletions(-) Approvals: Kudu Jenkins: Verified Jean-Daniel Cryans: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/8406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia75b65d66a1559ddaefb53dc1e67837faf9e7e6a Gerrit-Change-Number: 8406 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] python: upgrade pip when building client and restore old workarounds
Jean-Daniel Cryans has posted comments on this change. ( http://gerrit.cloudera.org:8080/8406 ) Change subject: python: upgrade pip when building client and restore old workarounds .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/8406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia75b65d66a1559ddaefb53dc1e67837faf9e7e6a Gerrit-Change-Number: 8406 Gerrit-PatchSet: 1 Gerrit-Owner: Adar DemboGerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 30 Oct 2017 16:02:18 + Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7053 to look at the new patch set (#35). Change subject: KUDU-2191 (2/n): Hive Metastore client .. KUDU-2191 (2/n): Hive Metastore client This patch lays the groundwork for integrating the Kudu catalog with the Hive MetaStore. The focus of this patch is a Kudu-specific C++ HMS client (hms_client.[h|cc]) in a new hms module. This client provides bindings for the Hive Metastore APIs that Kudu will use in follow-up commits. - Thrift has been added as a dependency, and a mechanism for performing Thrift codegen at compile time has been added (see FindThrift.cmake, based on FindProtobuf.cmake) - Bison has been added as a build-time dependency, because the system bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10. - Hive and Hadoop have been added to thirdparty as test-only dependencies. - A Hive MetaStore external mini server is included for testing. See mini_hms.[h|cc]. - The Kudu Metastore plugin is compiled from CMake as a standalone jar for loading into the HMS mini server. Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee --- M CMakeLists.txt M build-support/dist_test.py M build-support/iwyu/iwyu-filter.awk M build-support/run_dist_test.py A cmake_modules/FindJavaHome.cmake A cmake_modules/FindThrift.cmake A src/kudu/hms/CMakeLists.txt A src/kudu/hms/hive_metastore.thrift A src/kudu/hms/hms_client-test.cc A src/kudu/hms/hms_client.cc A src/kudu/hms/hms_client.h A src/kudu/hms/mini_hms.cc A src/kudu/hms/mini_hms.h M thirdparty/LICENSE.txt M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh M thirdparty/download-thirdparty.sh M thirdparty/vars.sh 18 files changed, 2,928 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/35 -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 35 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7053 to look at the new patch set (#34). Change subject: KUDU-2191 (2/n): Hive Metastore client .. KUDU-2191 (2/n): Hive Metastore client This patch lays the groundwork for integrating the Kudu catalog with the Hive MetaStore. The focus of this patch is a Kudu-specific C++ HMS client (hms_client.[h|cc]) in a new hms module. This client provides bindings for the Hive Metastore APIs that Kudu will use in follow-up commits. - Thrift has been added as a dependency, and a mechanism for performing Thrift codegen at compile time has been added (see FindThrift.cmake, based on FindProtobuf.cmake) - Bison has been added as a build-time dependency, because the system bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10. - Hive and Hadoop have been added to thirdparty as test-only dependencies. - A Hive MetaStore external mini server is included for testing. See mini_hms.[h|cc]. - The Kudu Metastore plugin is compiled from CMake as a standalone jar for loading into the HMS mini server. Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee --- M CMakeLists.txt M build-support/dist_test.py M build-support/iwyu/iwyu-filter.awk M build-support/run_dist_test.py A cmake_modules/FindJavaHome.cmake A cmake_modules/FindThrift.cmake A src/kudu/hms/CMakeLists.txt A src/kudu/hms/hive_metastore.thrift A src/kudu/hms/hms_client-test.cc A src/kudu/hms/hms_client.cc A src/kudu/hms/hms_client.h A src/kudu/hms/mini_hms.cc A src/kudu/hms/mini_hms.h M thirdparty/LICENSE.txt M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh M thirdparty/download-thirdparty.sh M thirdparty/vars.sh 18 files changed, 2,928 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/34 -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 34 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7053 to look at the new patch set (#33). Change subject: KUDU-2191 (2/n): Hive Metastore client .. KUDU-2191 (2/n): Hive Metastore client This patch lays the groundwork for integrating the Kudu catalog with the Hive MetaStore. The focus of this patch is a Kudu-specific C++ HMS client (hms_client.[h|cc]) in a new hms module. This client provides bindings for the Hive Metastore APIs that Kudu will use in follow-up commits. - Thrift has been added as a dependency, and a mechanism for performing Thrift codegen at compile time has been added (see FindThrift.cmake, based on FindProtobuf.cmake) - Bison has been added as a build-time dependency, because the system bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10. - Hive and Hadoop have been added to thirdparty as test-only dependencies. - A Hive MetaStore external mini server is included for testing. See mini_hms.[h|cc]. - The Kudu Metastore plugin is compiled from CMake as a standalone jar for loading into the HMS mini server. Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee --- M CMakeLists.txt M build-support/dist_test.py M build-support/iwyu/iwyu-filter.awk M build-support/run_dist_test.py A cmake_modules/FindJavaHome.cmake A cmake_modules/FindThrift.cmake A src/kudu/hms/CMakeLists.txt A src/kudu/hms/hive_metastore.thrift A src/kudu/hms/hms_client-test.cc A src/kudu/hms/hms_client.cc A src/kudu/hms/hms_client.h A src/kudu/hms/mini_hms.cc A src/kudu/hms/mini_hms.h M thirdparty/LICENSE.txt M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh M thirdparty/download-thirdparty.sh M thirdparty/vars.sh 18 files changed, 2,926 insertions(+), 6 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/33 -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 33 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7053 to look at the new patch set (#32). Change subject: KUDU-2191 (2/n): Hive Metastore client .. KUDU-2191 (2/n): Hive Metastore client This patch lays the groundwork for integrating the Kudu catalog with the Hive MetaStore. The focus of this patch is a Kudu-specific C++ HMS client (hms_client.[h|cc]) in a new hms module. This client provides bindings for the Hive Metastore APIs that Kudu will use in follow-up commits. - Thrift has been added as a dependency, and a mechanism for performing Thrift codegen at compile time has been added (see FindThrift.cmake, based on FindProtobuf.cmake) - Bison has been added as a build-time dependency, because the system bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10. - Hive and Hadoop have been added to thirdparty as test-only dependencies. - A Hive MetaStore external mini server is included for testing. See mini_hms.[h|cc]. - The Kudu Metastore plugin is compiled from CMake as a standalone jar for loading into the HMS mini server. Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee --- M CMakeLists.txt M build-support/dist_test.py M build-support/iwyu/iwyu-filter.awk M build-support/run_dist_test.py A cmake_modules/FindJavaHome.cmake A cmake_modules/FindThrift.cmake A src/kudu/hms/CMakeLists.txt A src/kudu/hms/hive_metastore.thrift A src/kudu/hms/hms_client-test.cc A src/kudu/hms/hms_client.cc A src/kudu/hms/hms_client.h A src/kudu/hms/mini_hms.cc A src/kudu/hms/mini_hms.h M thirdparty/LICENSE.txt M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh M thirdparty/download-thirdparty.sh M thirdparty/vars.sh 18 files changed, 2,924 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/32 -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 32 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 ) Change subject: KUDU-2191 (2/n): Hive Metastore client .. Patch Set 31: (1 comment) http://gerrit.cloudera.org:8080/#/c/7053/31//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/7053/31//COMMIT_MSG@4 PS31, Line 4: d...@danburkert.com revert -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 31 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 30 Oct 2017 14:56:46 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/7053 ) Change subject: KUDU-2191 (2/n): Hive Metastore client .. Patch Set 30: (14 comments) I've added TODOs to expand the error handling behavior and documentation in HmsClient. I'd like for these things to be tested thoroughly, but this commit is already quite large when considering the build changes, so I'd like to leave it to a follow-up commit. http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py File build-support/run_dist_test.py: http://gerrit.cloudera.org:8080/#/c/7053/30/build-support/run_dist_test.py@128 PS30, Line 128: env['HIVE_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/apache-hive-*-bin"))[0] : env['HADOOP_HOME'] = glob.glob(os.path.join(ROOT, "thirdparty/src/hadoop-*"))[0] > when we end up revving these dependencies it seems likely we'll end up with Done http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/CMakeLists.txt File src/kudu/hms/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/CMakeLists.txt@40 PS30, Line 40: execute_process(COMMAND ln -nsf : "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hive" : "${EXECUTABLE_OUTPUT_PATH}/hive-home") : execute_process(COMMAND ln -nsf : "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/opt/hadoop" : "${EXECUTABLE_OUTPUT_PATH}/hadoop-home") : execute_process(COMMAND ln -nsf : "${JAVA_HOME}" : "${EXECUTABLE_OUTPUT_PATH}/java-home") > does it make more sense to do these in build-thirdparty? even though it's n I don't think it's possible, since we can't know what directory the user will build Kudu in during the thirdparty build. http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hive_metastore.thrift File src/kudu/hms/hive_metastore.thrift: http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hive_metastore.thrift@21 PS30, Line 21: # DO NOT MODIFY! Copied from > should we be setting any C++ specific options like 'moveable types'? I added moveable_types to FindThrift generation. I don't think it really buys us much since all the service interfaces still takes args by const ref, but I guess it helps for building the arguments to begin with. http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client-test.cc File src/kudu/hms/hms_client-test.cc: http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client-test.cc@103 PS30, Line 103: ASSERT_TRUE(CreateTable(, database_name, table_name, "").IsRuntimeError()); > i think it's worth ASSERT_STR_MATCHES the error string here since RuntimeEr Done http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h File src/kudu/hms/hms_client.h: http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@60 PS30, Line 60: // can not be reached, an error is returned. > is there some default timeout, etc? Not currently, I've added a TODO. http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@73 PS30, Line 73: // returned. > it's probably worth a class-wide comment on what the errors returned will b I've added a TODO. I think we should have test coverage if we're going to document it, and I'd like to leave that to a follow-up commit. http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@85 PS30, Line 85: bool cascade = false, : bool delete_data = true) > would this be better off as a bitset of flags? Not in my opinion. What advantage would that have? This method will only be used in tests, per the current design. http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@121 PS30, Line 121:int32_t max_events, > do we have any idea of the expected size of these events? wondering if we n They can be very large for HDFS tables with a lot of partitions. There's currently no way to filter the events to just include Kudu tables, unfortunately. http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.h@124 PS30, Line 124: // Deserializes a JSON encoded table. > can you add a little more context as to where one would see the JSON-encode Done http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h File src/kudu/hms/hms_client.h: http://gerrit.cloudera.org:8080/#/c/7053/29/src/kudu/hms/hms_client.h@22 PS29, Line 22: #include > Could you at least add the IWYU pragma, please? It should be something lik sure, I completely forgot pragmas were an option. http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc File src/kudu/hms/hms_client.cc: http://gerrit.cloudera.org:8080/#/c/7053/30/src/kudu/hms/hms_client.cc@78 PS30, Line 78: IOError > would RemoteError
[kudu-CR] KUDU-2191 (2/n): Hive Metastore client
Hello Tidy Bot, Alexey Serbin, Jean-Daniel Cryans, Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7053 to look at the new patch set (#31). Change subject: KUDU-2191 (2/n): Hive Metastore client .. KUDU-2191 (2/n): Hive Metastore client This patch lays the groundwork for integrating the Kudu catalog with the Hive MetaStore. The focus of this patch is a Kudu-specific C++ HMS client (hms_client.[h|cc]) in a new hms module. This client provides bindings for the Hive Metastore APIs that Kudu will use in follow-up commits. - Thrift has been added as a dependency, and a mechanism for performing Thrift codegen at compile time has been added (see FindThrift.cmake, based on FindProtobuf.cmake) - Bison has been added as a build-time dependency, because the system bison version on RHEL 6 is not sufficiently new enough for Thrift 0.10. - Hive and Hadoop have been added to thirdparty as test-only dependencies. - A Hive MetaStore external mini server is included for testing. See mini_hms.[h|cc]. - The Kudu Metastore plugin is compiled from CMake as a standalone jar for loading into the HMS mini server. Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee --- M CMakeLists.txt M build-support/dist_test.py M build-support/iwyu/iwyu-filter.awk M build-support/run_dist_test.py A cmake_modules/FindJavaHome.cmake A cmake_modules/FindThrift.cmake A src/kudu/hms/CMakeLists.txt A src/kudu/hms/hive_metastore.thrift A src/kudu/hms/hms_client-test.cc A src/kudu/hms/hms_client.cc A src/kudu/hms/hms_client.h A src/kudu/hms/mini_hms.cc A src/kudu/hms/mini_hms.h M thirdparty/LICENSE.txt M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh M thirdparty/download-thirdparty.sh M thirdparty/vars.sh 18 files changed, 2,920 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/53/7053/31 -- To view, visit http://gerrit.cloudera.org:8080/7053 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I155223da912bc18a759df2f1f6bc25d1132a99ee Gerrit-Change-Number: 7053 Gerrit-PatchSet: 31 Gerrit-Owner: Dan BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon