[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7966 to look at the new patch set (#5). Change subject: KUDU-2134: Defer block transaction commit to the end of a tablet copy .. KUDU-2134: Defer block transaction commit to the end of a tablet copy KUDU-2131 uncovered a regression that causes a tablet copy session to expire reliably if the copied tablet is large and the tserver already has a high number of containers. Even though the issue is mitigated by LIFO container selection, we can completely avoid it by deferring the block transaction commit to the end of tablet copy seesion. There are mainly two reasons we may still want to do so: 1) If DownloadWALs fails there's no reason to pay the fsync() price and commit all those blocks. 2) While DownloadWALs is running the kernel has more time to eagerly flush the blocks, so the fsync()s at the end could be cheaper. This patch moves the block transaction commit out from FetchAll() and commits all downloaded blocks before replacing the superblock. Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b --- M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h 3 files changed, 40 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/7966/5 -- To view, visit http://gerrit.cloudera.org:8080/7966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2055[part 1]: Coalesce hole punching when deleting groups of blocks
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7656 to look at the new patch set (#4). Change subject: KUDU-2055[part 1]: Coalesce hole punching when deleting groups of blocks .. KUDU-2055[part 1]: Coalesce hole punching when deleting groups of blocks Similar to 'BlockCreationTransaction', this patch adds a new layer of abstraction at Block Manager to coalesce blocks deletions in a logical operation, e.g. compaction. By coalescing blocks deletions, the number of hole punching in LBM will be reduced from one per block to one per log block container. This should overall optimize the performance of operation that involves batch deletions, such as compaction and hole repunching. This patch is the first part of a serial. It only adds the new abstraction 'BlockDeletionTransaction', and doesn't yet change any deletion semantics or improve performance. Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959 --- M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/multi_column_writer.cc M src/kudu/tablet/multi_column_writer.h M src/kudu/tserver/tablet_copy_client.h 19 files changed, 225 insertions(+), 95 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7656/4 -- To view, visit http://gerrit.cloudera.org:8080/7656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Hello Todd Lipcon, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7988 to look at the new patch set (#2). Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup It is possible for tombstoned replicas to legitimately not have a cmeta file as a result of crashing during a first tablet copy, or failing a tablet copy operation in an older version of Kudu. Not having a cmeta file results in those tombstoned replicas being unable to vote in Raft leader elections. We remedy this by creating a cmeta object (with an empty config) at startup time. The empty config is safe for a tombstoned replica, because the config doesn't affect a replica's ability to vote in a leader election. Additionally, if the tombstoned replica were ever to be overwritten by a tablet copy operation, that would also result in overwriting the config stored in the local cmeta with a valid Raft config. Finally, all of this assumes that the nonexistence of a cmeta file guarantees that the replica has never voted in a leader election. As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE flag, meaning that it will only be flushed to disk if the replica ever votes. The following changes had to be made to ConsensusMetadata and the ConsensConsensusMetadataManager to support the above functionality: * Enable deferred flush on Create() by defining a flag called NO_FLUSH_ON_CREATE * Simplify the interface controlling whether a Flush() is allowed to overwrite (clobber) an existing file by encapsulating that logic in thethe implementation, instead of the public interface to ConsensusMetadata. When a cmeta is instantiated via ConsensusMetadata::Create(), the next Flush() is not allowed to overwrite an existing file. In every other case, Flush() is allowed to overwrite an existing file. The following tests have been added: * A unit test for NO_FLUSH_ON_CREATE. * A test that crashes the target of a tablet copy after writing the superblock and before writing the cmeta file. The tablet server is restarted and the replica is expected to be able to vote while tombstoned. Previously-written tests that verify ConsensusMetadata::Create() will not clobber an existing file still pass. Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b --- M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/consensus_meta_manager-stress-test.cc M src/kudu/consensus/consensus_meta_manager-test.cc M src/kudu/consensus/consensus_meta_manager.cc M src/kudu/consensus/consensus_meta_manager.h M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/tombstoned_voting-itest.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc 16 files changed, 357 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/7988/2 -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-test
Hello Todd Lipcon, Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7991 to review the following change. Change subject: Rename tombstoned_voting-itest to tombstoned_voting-test .. Rename tombstoned_voting-itest to tombstoned_voting-test tombstoned_voting-itest uses an InternalMiniCluster, and it would be useful to have tombstoned voting tests that use an ExternalMiniCluster. Rename the existing IMC-based test to -test prefix to make room for an EMC-based test that has an -itest prefix. Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06 --- M src/kudu/integration-tests/CMakeLists.txt R src/kudu/integration-tests/tombstoned_voting-test.cc 2 files changed, 10 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/7991/1 -- To view, visit http://gerrit.cloudera.org:8080/7991 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Andrew Wong has uploaded a new change for review. http://gerrit.cloudera.org:8080/7992 Change subject: KUDU-2138 delete failed replicas in tablet report .. KUDU-2138 delete failed replicas in tablet report When a Kudu master processes a replica's tablet report, it will send a DeleteTablet RPC if the replica is not already deleted, has a consensus state, has a lower opid index than the latest reported committed config, and is no longer in the tablet's Raft configuration. Failed tablets are handled specially in processing the report to return before any of the above checks can be made. This can lead to failed tablets lingering when they should actually be deleted. This patch fixes this by performing these checks and sending the delete regardless of whether or not the tablet is failed (other than checking the opid index, since failed tablets do not report an opid index). A test is added to tablet_replacement-itest to fail a replica, evict but not tombstone it, report it to the master, and ensure it is properly deleted. Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c --- M src/kudu/integration-tests/tablet_replacement-itest.cc M src/kudu/master/catalog_manager.cc 2 files changed, 109 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7992/1 -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Andrew Wong has uploaded a new patch set (#2). Change subject: KUDU-2138 delete failed replicas in tablet report .. KUDU-2138 delete failed replicas in tablet report When a Kudu master processes a replica's tablet report, it will send a DeleteTablet RPC if the replica is not already deleted, has a consensus state, has a lower opid index than the latest reported committed config, and is no longer in the tablet's Raft configuration. Failed tablets are handled specially in processing the report to return before any of the above checks can be made. This can lead to failed tablets lingering when they should actually be deleted. This patch fixes this by performing these checks and sending the delete regardless of whether or not the tablet is failed (other than checking the opid index, since failed tablets do not report an opid index). A test is added to tablet_replacement-itest to fail a replica, evict but not tombstone it, report it to the master, and ensure it is properly deleted. Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c --- M src/kudu/integration-tests/tablet_replacement-itest.cc M src/kudu/master/catalog_manager.cc 2 files changed, 104 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7992/2 -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Andrew Wong has posted comments on this change. Change subject: KUDU-2138 delete failed replicas in tablet report .. Patch Set 2: IWYU failure; this patch is still reviewable -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Adar Dembo has posted comments on this change. Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. Patch Set 2: (4 comments) Was just passing through because I was interested. http://gerrit.cloudera.org:8080/#/c/7988/2//COMMIT_MSG Commit Message: PS2, Line 7: replicas Nit: replica ("doesn't" implies singular) PS2, Line 11: Not having a cmeta : file results in those tombstoned replicas being unable to vote in Raft : leader elections Given the conditions you spelled out in the previous sentence, it seems like not having a cmeta would be quite rare. Why is it important for these replicas to be able to vote? PS2, Line 27: ConsensConsensusMetadataManager ConsensusMetadataManager PS2, Line 33: thethe the -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced
Andrew Wong has uploaded a new change for review. http://gerrit.cloudera.org:8080/7993 Change subject: Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced .. Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced TestFailedTabletsAreReplaced manually fails the replica after only verifying that the tablet exists, with no regard for its state. This can cause the replica's bootstrap process to fail a check: F0907 00:05:46.153576 2697 tablet_replica.cc:173] Check failed: BOOTSTRAPPING == state_ (0 vs. 2) This is a test-only race where the replica successfully goes through the bootstrap process, the tablet is failed in test, and the TabletReplica::Start() is called on the replica, which requires its state to be BOOTSTRAPPING. This is not an issue seen in production, as bootstrapping is normally only run if the replica is not failed, but it did result in 6/1000 failures when run in release mode with --stress_cpu_thres=32. To fix this, the replica is failed only after the it is verified to be running. In doing so, the number of failures went from 6/1000 to 0/1000. Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc 1 file changed, 14 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/7993/1 -- To view, visit http://gerrit.cloudera.org:8080/7993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong
[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced
Adar Dembo has posted comments on this change. Change subject: Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced .. Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/7993/1//COMMIT_MSG Commit Message: PS1, Line 15: the Nit: drop this PS1, Line 22: the Nit: drop http://gerrit.cloudera.org:8080/#/c/7993/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc File src/kudu/integration-tests/ts_tablet_manager-itest.cc: Line 146: AssertEventually([&]{ When using this form of AssertEventually you need to manually call NO_PENDING_FATALS() after it. Line 150: }, MonoDelta::FromSeconds(60)); Is it important to override the default timeout value of 30s? Line 153: { Nit: add a blank line just before this. -- To view, visit http://gerrit.cloudera.org:8080/7993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Adar Dembo has posted comments on this change. Change subject: KUDU-2138 delete failed replicas in tablet report .. Patch Set 2: Code-Review+1 (2 comments) Looks OK, but would prefer if Mike +2'ed. http://gerrit.cloudera.org:8080/#/c/7992/2/src/kudu/integration-tests/tablet_replacement-itest.cc File src/kudu/integration-tests/tablet_replacement-itest.cc: PS2, Line 184: an a http://gerrit.cloudera.org:8080/#/c/7992/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 2590: if (report.has_error()) { I'm curious as to why this block was moved down here and not just before the large "if report.has_consensus_state()" block? I can take my answer in the form of a code comment. -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7993 to look at the new patch set (#2). Change subject: Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced .. Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced TestFailedTabletsAreReplaced manually fails the replica after only verifying that the tablet exists, with no regard for its state. This can cause the replica's bootstrap process to fail a check: F0907 00:05:46.153576 2697 tablet_replica.cc:173] Check failed: BOOTSTRAPPING == state_ (0 vs. 2) This is a test-only race where the replica successfully goes through the bootstrap process, the tablet is failed in test, and TabletReplica::Start() is called on the replica, which requires its state to be BOOTSTRAPPING. This is not an issue seen in production, as bootstrapping is normally only run if the replica is not failed, but it did result in 6/1000 failures when run in release mode with --stress_cpu_thres=32. To fix this, the replica is failed only after it is verified to be running. In doing so, the number of failures went from 6/1000 to 0/1000. Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc 1 file changed, 16 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/93/7993/2 -- To view, visit http://gerrit.cloudera.org:8080/7993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced
Andrew Wong has posted comments on this change. Change subject: Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced .. Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/7993/1//COMMIT_MSG Commit Message: PS1, Line 15: > Nit: drop this Done PS1, Line 22: it > Nit: drop Done http://gerrit.cloudera.org:8080/#/c/7993/1/src/kudu/integration-tests/ts_tablet_manager-itest.cc File src/kudu/integration-tests/ts_tablet_manager-itest.cc: Line 146: AssertEventually([&]{ > When using this form of AssertEventually you need to manually call NO_PENDI Done Line 150: }, MonoDelta::FromSeconds(60)); > Is it important to override the default timeout value of 30s? I ran into 3/1000 failures when it was 30s (couldn't get to RUNNING because it was still bootstrapping or copying, not the flake failure). Line 153: wait_until_running(); > Nit: add a blank line just before this. Done -- To view, visit http://gerrit.cloudera.org:8080/7993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7992 to look at the new patch set (#3). Change subject: KUDU-2138 delete failed replicas in tablet report .. KUDU-2138 delete failed replicas in tablet report When a Kudu master processes a replica's tablet report, it will send a DeleteTablet RPC if the replica is not already deleted, has a consensus state, has a lower opid index than the latest reported committed config, and is no longer in the tablet's Raft configuration. Failed tablets are handled specially in processing the report to return before any of the above checks can be made. This can lead to failed tablets lingering when they should actually be deleted. This patch fixes this by performing these checks and sending the delete regardless of whether or not the tablet is failed (other than checking the opid index, since failed tablets do not report an opid index). A test is added to tablet_replacement-itest to fail a replica, evict but not tombstone it, report it to the master, and ensure it is properly deleted. Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c --- M src/kudu/integration-tests/tablet_replacement-itest.cc M src/kudu/master/catalog_manager.cc 2 files changed, 101 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7992/3 -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Andrew Wong has posted comments on this change. Change subject: KUDU-2138 delete failed replicas in tablet report .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/7992/2/src/kudu/integration-tests/tablet_replacement-itest.cc File src/kudu/integration-tests/tablet_replacement-itest.cc: PS2, Line 184: an > a Done http://gerrit.cloudera.org:8080/#/c/7992/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 2590: if (report.has_error()) { > I'm curious as to why this block was moved down here and not just before th I'd wanted it to be slightly tailored for the case I expect is more common where report.has_consensus_state() == true so I put it first. It's not really an important move, so I'll move it back, sorry about that. -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] docs: update disk failure recovery notes
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7984 to look at the new patch set (#2). Change subject: docs: update disk failure recovery notes .. docs: update disk failure recovery notes Adds more context around disk failures and separates out steps to rebuild a server with a different directory configuration. Change-Id: I732286d0f56f7a15705ad544fc7dfc426287714e --- M docs/administration.adoc 1 file changed, 38 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/7984/2 -- To view, visit http://gerrit.cloudera.org:8080/7984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I732286d0f56f7a15705ad544fc7dfc426287714e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] docs: update disk failure recovery notes
Andrew Wong has posted comments on this change. Change subject: docs: update disk failure recovery notes .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/7984/1/docs/administration.adoc File docs/administration.adoc: Line 597: empty all of the server's existing directories. For example, if a tablet server > do you think a WARNING bar is appropriate here? I wonder if someone would f Done PS1, Line 608: along with any newly-added data : directories > you mean 'and new data directories have been created with appropriate permi Done PS1, Line 630: suicide_on_eio=false`. When set, tablets with data on a failed disk : will not be opened and will be re-replicated as necessary > given that we are currently defaulting to spreading tablets across all disk Fair point about striping. Made it less of a point. Also changed to --crash_on_eio; hopefully less controversial and made this patch dependent on the separate patch. -- To view, visit http://gerrit.cloudera.org:8080/7984 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I732286d0f56f7a15705ad544fc7dfc426287714e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] rename suicide on eio flag to crash on eio
Andrew Wong has uploaded a new change for review. http://gerrit.cloudera.org:8080/7994 Change subject: rename suicide_on_eio flag to crash_on_eio .. rename suicide_on_eio flag to crash_on_eio The name `suicide_on_eio` seems a bit controversial/insensitive. As we move forward in documenting its usage with disk failures, it's probably best rename it. Change-Id: I6c83156865b0f8ac80a20701558119ace6c05399 --- M docs/release_notes.adoc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/log_block_manager-test.cc M src/kudu/integration-tests/disk_failure-itest.cc M src/kudu/util/env-test.cc M src/kudu/util/env_posix.cc 8 files changed, 17 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/7994/1 -- To view, visit http://gerrit.cloudera.org:8080/7994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I6c83156865b0f8ac80a20701558119ace6c05399 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong
[kudu-CR] rename suicide on eio flag to crash on eio
Adar Dembo has posted comments on this change. Change subject: rename suicide_on_eio flag to crash_on_eio .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7994 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6c83156865b0f8ac80a20701558119ace6c05399 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced
Adar Dembo has posted comments on this change. Change subject: Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced .. Patch Set 2: Code-Review+1 Will defer to Mike. -- To view, visit http://gerrit.cloudera.org:8080/7993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] KUDU-2137: protect against concurrent schema version change and tablet drop
Hello Dan Burkert, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7996 to review the following change. Change subject: KUDU-2137: protect against concurrent schema version change and tablet drop .. KUDU-2137: protect against concurrent schema version change and tablet drop Try as I might, I could not reproduce the failure in the bug report. I looped the failed test several thousand times. I also looped the entire alter_table-test suite a thousand times. Finally, I wrote a unit test that hammers one table with concurrent add column, add partition, and drop partition operations. Nothing worked. So, here's my best guess at what's going on: if a tablet is dropped while the master is processing its report, it's conceivable that we could wind up in TabletInfo::set_reported_schema_version() with the table spinlock held just after TableInfo::AddRemoveTablets() dropped the tablet. This would cause us to decrement the tablet's "old" schema version from the table's count map twice: once when dropping the tablet and a second time in set_reported_schema_version(). The fix is straight-forward: after acquiring both spinlocks, double check that the tablet is still a member of the table. But, the extra locking needed to do so feels so very wrong. Change-Id: I371fc310a97ae94ec2ebf04405db99c5f2937e1a --- M src/kudu/master/catalog_manager.cc 1 file changed, 16 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/7996/1 -- To view, visit http://gerrit.cloudera.org:8080/7996 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I371fc310a97ae94ec2ebf04405db99c5f2937e1a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] catalog manager: fix unprotected data access in TableInfo::AddRemoveTablets
Hello Dan Burkert, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7995 to review the following change. Change subject: catalog_manager: fix unprotected data access in TableInfo::AddRemoveTablets .. catalog_manager: fix unprotected data access in TableInfo::AddRemoveTablets RWCLock::HasWriteLock never worked properly because last_writer_tid_ wasn't reset when the lock was released. Well, it worked properly (though it's far less useful) in non-DEBUG builds, but then the various HasWriteLock DCHECKs are compiled out. Who knew? When fixed, TableInfo::AddRemoveTablets broke; we had been reading some tablet metadata without first acquiring a lock! This was frustrasting to address because it meant retreading over the most error-prone aspect of the catalog manager: for operations that require several writes in order to "publish" their results, in what order should those writes occur? I tried my best to get this right, but who knows how I did... Change-Id: Ifcc5d3a9b985210f1fdd6f0495326fa3eb707841 --- M src/kudu/master/catalog_manager-test.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/catalog_manager.h M src/kudu/util/cow_object.h M src/kudu/util/rwc_lock.cc M src/kudu/util/rwc_lock.h 6 files changed, 60 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/95/7995/1 -- To view, visit http://gerrit.cloudera.org:8080/7995 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ifcc5d3a9b985210f1fdd6f0495326fa3eb707841 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Add tablet state summary metrics
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7980 to look at the new patch set (#5). Change subject: Add tablet state summary metrics .. Add tablet state summary metrics This patch adds metrics for the number of tablets in each state (i.e. the number of tablets for each value of TabletStatePB). To avoid contention in the tablet manager, the frequency with which the state numbers are computed is limited. Requests that come too soon for an update will receive cached counts. The maximum frequency of updates is controlled by a new flag. Change-Id: I9fda2061d341586f0aa343787af59984a627080a --- M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/util/metrics.cc M src/kudu/util/metrics.h 5 files changed, 308 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/7980/5 -- To view, visit http://gerrit.cloudera.org:8080/7980 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7981 to look at the new patch set (#4). Change subject: KUDU-2044 Tombstoned tablets show up in /metrics .. KUDU-2044 Tombstoned tablets show up in /metrics This patch stops tombstoned tablets from showing up in /metrics. When a tablet replica is shut down, the tablet's metric entity is marked as unpublished. All its child metrics will be marked for retirement, then retired after the retirement interval has passed. Then the entity itself is unregistered, i.e. removed from the metric registry's map of entities. If a new replica of the same tablet is added to the server, it will create a new entity that will be registered with the metric_registry, either as a new insertion or overwriting the old replica's entity if the entity had been unpublished but not yet unregistered. The tombstoned's tablets metric entity is not destroyed when it's deregistered, since there are still refs to it and its metrics in the TabletReplica class, Tablet class, and many other classes associated with a TabletReplica. The entity will be destroyed when the TabletReplica is deleted (since it either contains or holds final references to all the other classes), which happens if the tablet is replaced or deleted. While it's not ideal to hold the memory for the entity when it's no longer used, the reason this occurs is because the TabletReplica instance mostly stays alive. Releasing all the metric references would require specifically dropping refs to those metrics in all the surviving subcomponents of a TabletReplica instance that has been shut down; I think this problem would be better solved by more completely cleaning up a shut down TabletReplica instance, but that's a much larger scope than suppressing the metrics. Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3 --- M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/tablet/tablet.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/util/metrics.cc M src/kudu/util/metrics.h 5 files changed, 144 insertions(+), 8 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/7981/4 -- To view, visit http://gerrit.cloudera.org:8080/7981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley
[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics
Will Berkeley has posted comments on this change. Change subject: KUDU-2044 Tombstoned tablets show up in /metrics .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/7981/3//COMMIT_MSG Commit Message: PS3, Line 10: it > is Done http://gerrit.cloudera.org:8080/#/c/7981/3/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: PS3, Line 2582: do > go? Done -- To view, visit http://gerrit.cloudera.org:8080/7981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] Fix typo in partitioning error message
Hello Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/7997 to review the following change. Change subject: Fix typo in partitioning error message .. Fix typo in partitioning error message Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866 --- M src/kudu/common/partition.cc 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/7997/1 -- To view, visit http://gerrit.cloudera.org:8080/7997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Alexey Serbin has uploaded a new change for review. http://gerrit.cloudera.org:8080/7998 Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. [iwyu] kudu-specific mappings for std::tr1 entities Added Kudu-specific IWYU mappings for std::tr1::shared_ptr and friends from TR1 (those are used in the Kudu C++ client API). Also, moved the instructions for running a non-incremental IWYU verification (i.e. running the tool against every C++ source file in the project) from build-support/iwyu/iwyu-filter.awk file into top-level README.adoc The additional --max_line_length=256 IWYU option allows for better reporting of the reason for header files inclusion. If keeping the default setting of 80 characters, IWYU sometimes skipped the information which symbols require particular header file to be included. Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 --- M .gitignore M README.adoc M build-support/iwyu/iwyu-filter.awk M build-support/iwyu/iwyu.sh A build-support/iwyu/mappings/kudu.imp 5 files changed, 87 insertions(+), 46 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/7998/1 -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7988 to look at the new patch set (#3). Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup It is possible for tombstoned replicas to legitimately not have a cmeta file as a result of crashing during a first tablet copy, or failing a tablet copy operation in an older version of Kudu. Not having a cmeta file results in those tombstoned replicas being unable to vote in Raft leader elections. We remedy this by creating a cmeta object (with an empty config) at startup time. The empty config is safe for a tombstoned replica, because the config doesn't affect a replica's ability to vote in a leader election. Additionally, if the tombstoned replica were ever to be overwritten by a tablet copy operation, that would also result in overwriting the config stored in the local cmeta with a valid Raft config. Finally, all of this assumes that the nonexistence of a cmeta file guarantees that the replica has never voted in a leader election. As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE flag, meaning that it will only be flushed to disk if the replica ever votes. The following changes had to be made to ConsensusMetadata and the ConsensusMetadataManager to support the above functionality: * Enable deferred flush on Create() by defining a flag called NO_FLUSH_ON_CREATE * Simplify the interface controlling whether a Flush() is allowed to overwrite (clobber) an existing file by encapsulating that logic in the implementation, instead of the public interface to ConsensusMetadata. When a cmeta is instantiated via ConsensusMetadata::Create(), the next Flush() is not allowed to overwrite an existing file. In every other case, Flush() is allowed to overwrite an existing file. The following tests have been added: * A unit test for NO_FLUSH_ON_CREATE. * A test that crashes the target of a tablet copy after writing the superblock and before writing the cmeta file. The tablet server is restarted and the replica is expected to be able to vote while tombstoned. Previously-written tests that verify ConsensusMetadata::Create() will not clobber an existing file still pass. Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b --- M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/consensus_meta_manager-stress-test.cc M src/kudu/consensus/consensus_meta_manager-test.cc M src/kudu/consensus/consensus_meta_manager.cc M src/kudu/consensus/consensus_meta_manager.h M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/tombstoned_voting-itest.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc 16 files changed, 357 insertions(+), 38 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/7988/3 -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Mike Percy has posted comments on this change. Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/7988/2//COMMIT_MSG Commit Message: PS2, Line 7: replicas > Nit: replica If the cmeta doesn't exist PS2, Line 11: Not having a cmeta : file results in those tombstoned replicas being unable to vote in Raft : leader elections > Given the conditions you spelled out in the previous sentence, it seems lik Due to previous versions of Kudu generating many of these kinds of tombstoned replicas under load, it's not that rare. Additionally, we don't want them to show up as FAILED in the UI, so not making them as FAILED was the primary motivation, and being able to vote is really something of a side benefit. PS2, Line 27: ConsensConsensusMetadataManager > ConsensusMetadataManager erg, my Vim auto-wrapping keeps doing this in Git messages. I need to figure out why, just haven't got around to it. PS2, Line 33: thethe > the same, thanks -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced
Hao Hao has posted comments on this change. Change subject: Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced .. Patch Set 2: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] Fix typo in partitioning error message
Hao Hao has posted comments on this change. Change subject: Fix typo in partitioning error message .. Patch Set 1: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] Fix typo in partitioning error message
Alexey Serbin has posted comments on this change. Change subject: Fix typo in partitioning error message .. Patch Set 1: It seems src/kudu/master/master-test.cc should be also updated. -- To view, visit http://gerrit.cloudera.org:8080/7997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Alexey Serbin has posted comments on this change. Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. Patch Set 1: Verified+1 Unrelated flake in RaftConsensusITest.TestTombstonedVoteAfterFailedTabletCopy -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-HasComments: No
[kudu-CR] Fix typo in partitioning error message
Hello Hao Hao, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7997 to look at the new patch set (#2). Change subject: Fix typo in partitioning error message .. Fix typo in partitioning error message Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866 --- M src/kudu/common/partition.cc M src/kudu/master/master-test.cc 2 files changed, 3 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/97/7997/2 -- To view, visit http://gerrit.cloudera.org:8080/7997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Alexey Serbin has posted comments on this change. Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. Patch Set 3: (9 comments) Just some nits for a while -- will take a deeper 'semantic' look shortly. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager-stress-test.cc File src/kudu/consensus/consensus_meta_manager-stress-test.cc: PS3, Line 18: #include nit: prefer using I'm planning to update IWYU to give more straightforward suggestions. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager.h File src/kudu/consensus/consensus_meta_manager.h: PS3, Line 59: ConsensusMetadataCreateMode create_mode What if this parameter was set to ConsensusMetadataCreateMode::FLUSH_ON_CREATE by default? That would allow for removal of some LOC in consensus_meta_manager-test.cc and other places where FLUS_ON_CREATE is the desired behavior and the cmeta_out is not needed. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/raft_consensus_quorum-test.cc File src/kudu/consensus/raft_consensus_quorum-test.cc: PS3, Line 191: scoped_refptr cmeta; nit: drop this and leave the last parameter of the Create() call below by default. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/integration-tests/tombstoned_voting-itest.cc File src/kudu/integration-tests/tombstoned_voting-itest.cc: PS3, Line 18: #include nit: consider replacing with and adding that into the list of the STD headers just below. I'm sorry for the inconvenience right now, but I'm planning to update IWYU-related stuff accordingly to make its suggestions more straightforward. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: PS3, Line 260: scoped_refptr cmeta; nit: consider dropping this since it's not used in the scope and leaving the last parameter of the Create() call below by default. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tablet/tablet_bootstrap-test.cc File src/kudu/tablet/tablet_bootstrap-test.cc: PS3, Line 146: scoped_refptr cmeta; Nit: it seems this is not used, consider dropping it and leaving the last parameter for the Create() call below by default. Also, as mentioned elsewhere, consider having the second-from-the-tail parameter for Create() by default as well. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tablet/tablet_replica-test.cc File src/kudu/tablet/tablet_replica-test.cc: PS3, Line 141: scoped_refptr cmeta; Nit: consider dropping this since it's not used, leaving the last parameter for the Create() call below at its default value. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: PS3, Line 601: scoped_refptr cmeta; Nit: cmeta is not used in this scope, right? Consider dropping it. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tserver/tablet_copy_source_session-test.cc File src/kudu/tserver/tablet_copy_source_session-test.cc: PS3, Line 157: scoped_refptr cmeta; nit: consider dropping cmeta -- it's not used in this scope elsewhere but passing as the last parameter to the Create() call below, where it can be left by default. -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy
Dan Burkert has posted comments on this change. Change subject: KUDU-2134: Defer block transaction commit to the end of a tablet copy .. Patch Set 5: Code-Review+1 (2 comments) Looks good, I like how this makes the code a bit simpler (less parameters passed around). http://gerrit.cloudera.org:8080/#/c/7966/5//COMMIT_MSG Commit Message: PS5, Line 14: seesion session http://gerrit.cloudera.org:8080/#/c/7966/5/src/kudu/tserver/tablet_copy_client-test.cc File src/kudu/tserver/tablet_copy_client-test.cc: Line 180: ASSERT_OK(client_->transaction_.CommitCreatedBlocks()); Should these be calling Finish instead of reaching into the transaction manually? -- To view, visit http://gerrit.cloudera.org:8080/7966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Dan Burkert has posted comments on this change. Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7998/1/README.adoc File README.adoc: Line 292: Running include-what-you-use (IWYU) against all source files in project Is this going to be done commonly? If not maybe it would be better just to link to the previous location of the documentation. -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-HasComments: Yes
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Andrew Wong has posted comments on this change. Change subject: KUDU-2138 delete failed replicas in tablet report .. Patch Set 4: Just fixed IWYU failure, still good for review. -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Hello Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7992 to look at the new patch set (#4). Change subject: KUDU-2138 delete failed replicas in tablet report .. KUDU-2138 delete failed replicas in tablet report When a Kudu master processes a replica's tablet report, it will send a DeleteTablet RPC if the replica is not already deleted, has a consensus state, has a lower opid index than the latest reported committed config, and is no longer in the tablet's Raft configuration. Failed tablets are handled specially in processing the report to return before any of the above checks can be made. This can lead to failed tablets lingering when they should actually be deleted. This patch fixes this by performing these checks and sending the delete regardless of whether or not the tablet is failed (other than checking the opid index, since failed tablets do not report an opid index). A test is added to tablet_replacement-itest to fail a replica, evict but not tombstone it, report it to the master, and ensure it is properly deleted. Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c --- M src/kudu/integration-tests/tablet_replacement-itest.cc M src/kudu/master/catalog_manager.cc 2 files changed, 102 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7992/4 -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Alexey Serbin has posted comments on this change. Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7998/1/README.adoc File README.adoc: Line 292: Running include-what-you-use (IWYU) against all source files in project > Is this going to be done commonly? If not maybe it would be better just to I don't think it is, but I didn't find a better place to put it. I thought that keeping these notes in the awk script itself was not the best place. Maybe, I should put that a dedicated README.doc under build-support/iwyu? -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-HasComments: Yes
[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-test
Alexey Serbin has posted comments on this change. Change subject: Rename tombstoned_voting-itest to tombstoned_voting-test .. Patch Set 1: (2 comments) I though the '-itest' suffix stood for 'integration test'. Maybe, rename the file to reflect the usage of the IMC via adding that into the suffix instead? E.g., tombstoned_voting-itest.cc --> tombstoned_voting-imc-itest.cc ? http://gerrit.cloudera.org:8080/#/c/7991/1//COMMIT_MSG Commit Message: PS1, Line 11: prefix suffix? PS1, Line 12: prefix suffix? -- To view, visit http://gerrit.cloudera.org:8080/7991 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Fix typo in partitioning error message
Alexey Serbin has submitted this change and it was merged. Change subject: Fix typo in partitioning error message .. Fix typo in partitioning error message Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866 Reviewed-on: http://gerrit.cloudera.org:8080/7997 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/common/partition.cc M src/kudu/master/master-test.cc 2 files changed, 3 insertions(+), 3 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Fix typo in partitioning error message
Alexey Serbin has posted comments on this change. Change subject: Fix typo in partitioning error message .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7997 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5502956e7947a67dba3e68e5add529c7b9e26866 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Dan Burkert has posted comments on this change. Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7998/1/README.adoc File README.adoc: Line 292: Running include-what-you-use (IWYU) against all source files in project > I don't think it is, but I didn't find a better place to put it. I thought I'm fine with keeping it in the awk script and linking to that. This seems like something that we'll want to automate if it ends up being used a lot, at which point the directions would become significantly simpler. A separate README is fine with me if you feel that's better, though. -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-HasComments: Yes
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Andrew Wong has posted comments on this change. Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: PS3, Line 339: // Create() should not clobber. I think the placement of this comment might be a bit confusing. Should this comment be above L333, since that's where overwrite_first_flush is specified? http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager.cc File src/kudu/consensus/consensus_meta_manager.cc: PS3, Line 97: if (s.ok()) Did you consider narrowing the permissible errors here? e.g. if we hit an IO error when we Load(), but successfully create the in-memory cmeta, we can get by without addressing the error at all until the next flush. I suppose this probably won't affect correctness, but it might be worth thinking about what errors we can expect here, if you haven't already. -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Alexey Serbin has uploaded a new patch set (#2). Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. [iwyu] kudu-specific mappings for std::tr1 entities Added Kudu-specific IWYU mappings for std::tr1::shared_ptr and friends from TR1 (those are used in the Kudu C++ client API). Added --max_line_length=256 IWYU option for better reporting on the reason for header files inclusion. If keeping the default setting of 80 characters, IWYU sometimes omits the information which symbols require particular header file to be included. Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 --- M .gitignore M build-support/iwyu/iwyu-filter.awk M build-support/iwyu/iwyu.sh A build-support/iwyu/mappings/kudu.imp 4 files changed, 55 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/7998/2 -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Alexey Serbin has posted comments on this change. Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/7998/1/README.adoc File README.adoc: Line 292: Running include-what-you-use (IWYU) against all source files in project > I'm fine with keeping it in the awk script and linking to that. This seems SGTM -- I returned the instructions back into iwyu-filter.awk and updated them a little. -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy
Hao Hao has posted comments on this change. Change subject: KUDU-2134: Defer block transaction commit to the end of a tablet copy .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7966/5/src/kudu/tserver/tablet_copy_client-test.cc File src/kudu/tserver/tablet_copy_client-test.cc: Line 180: ASSERT_OK(client_->transaction_.CommitCreatedBlocks()); > Should these be calling Finish instead of reaching into the transaction man Finish() has some other logic which changes the state of tablet copy. I was trying to not include those that may effect the behavior of the tests. Does that make sense? -- To view, visit http://gerrit.cloudera.org:8080/7966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] block id: use a better hash function
Hello Dan Burkert, Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8001 to review the following change. Change subject: block_id: use a better hash function .. block_id: use a better hash function In looking at LBM startup time I noticed that the insertion into the block hashmap was taking substantially longer than advertised in various benchmarks. That led me to think that our block ID patterns were causing high collision rates. Swapping out the hash function for block IDs reduced the startup time substantially. Tested on a host with ~11M blocks across 14 drives using 'fs check'. Before: I0907 13:01:46.997755 21274 fs_manager.cc:335] Time spent opening block manager: real 114.501s user 0.000s sys 0.002s After: I0907 12:58:42.863929 20320 fs_manager.cc:335] Time spent opening block manager: real 69.951s user 0.001s sys 0.001s Change-Id: I30717955f962957d109a6403b55d59ab6c446a87 --- M src/kudu/fs/block_id.h 1 file changed, 7 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/01/8001/1 -- To view, visit http://gerrit.cloudera.org:8080/8001 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I30717955f962957d109a6403b55d59ab6c446a87 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert
[kudu-CR] ref counted: fix move constructors to actually move
Hello Dan Burkert, Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8002 to review the following change. Change subject: ref_counted: fix move constructors to actually move .. ref_counted: fix move constructors to actually move Ever since the move constructors for scoped_refptr were first implemented, they didn't properly move. That is to say, they caused an unnecessary AddRef/Release pair. The issue is that they were passing through an rvalue-reference parameter without properly using std::move(). See [1] for an explanation of this subtlety. When combined with some other in-flight patches, this resulted in a substantial improvement in LBM startup time, since a hashmap containing ref-counted elements could relocate them without unnecessary reference count increments and decrements. It's likely it will also improve performance elsewhere throughout Kudu. This patch also pulls in an extra move constructor from the Chromium code base which they claim helps Clang generate better warnings. I didn't verify that, but figured I'd copy it while I was looking at the code. [1] https://stackoverflow.com/questions/14486367/why-do-you-use-stdmove-when-you-have-in-c11 Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6 --- M src/kudu/gutil/ref_counted.h 1 file changed, 11 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/8002/1 -- To view, visit http://gerrit.cloudera.org:8080/8002 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Dan Burkert has posted comments on this change. Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Adar Dembo has posted comments on this change. Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/7998/2/.gitignore File .gitignore: Line 33: README.html What's this about? Do you convert README.md to README.html using a tool? http://gerrit.cloudera.org:8080/#/c/7998/2/build-support/iwyu/iwyu-filter.awk File build-support/iwyu/iwyu-filter.awk: Line 47: # IWYU="`pwd`/../../thirdparty/clang-toolchain/bin/include-what-you-use; \ Do you want to add --max_line_length here too? -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Adar Dembo has posted comments on this change. Change subject: KUDU-2138 delete failed replicas in tablet report .. Patch Set 4: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Alexey Serbin has posted comments on this change. Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/7998/2/.gitignore File .gitignore: Line 33: README.html > What's this about? Do you convert README.md to README.html using a tool? Yep. Do you think it's not worth adding this into the .gitignore? http://gerrit.cloudera.org:8080/#/c/7998/2/build-support/iwyu/iwyu-filter.awk File build-support/iwyu/iwyu-filter.awk: Line 47: # IWYU="`pwd`/../../thirdparty/clang-toolchain/bin/include-what-you-use; \ > Do you want to add --max_line_length here too? Yep, that would be good, thanks. -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Hello Dan Burkert, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7998 to look at the new patch set (#3). Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. [iwyu] kudu-specific mappings for std::tr1 entities Added Kudu-specific IWYU mappings for std::tr1::shared_ptr and friends from TR1 (those are used in the Kudu C++ client API). Added --max_line_length=256 IWYU option for better reporting on the reason for header files inclusion. If keeping the default setting of 80 characters, IWYU sometimes omits the information which symbols require particular header file to be included. Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 --- M .gitignore M build-support/iwyu/iwyu-filter.awk M build-support/iwyu/iwyu.sh A build-support/iwyu/mappings/kudu.imp 4 files changed, 56 insertions(+), 18 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/98/7998/3 -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2124. Don't hold session lock while initializing a TabletCopySession
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7985 to look at the new patch set (#3). Change subject: KUDU-2124. Don't hold session lock while initializing a TabletCopySession .. KUDU-2124. Don't hold session lock while initializing a TabletCopySession This patch replaces the interior mutex in TabletCopySourceSession with a dynamic once, so that initialization can happen concurrently. This allows initialization to happen outside of the critical section in the tablet copy service. Implemented a regression test that injects latency into BeginTabletCopySession() calls. Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91 --- M src/kudu/integration-tests/cluster_itest_util.cc M src/kudu/integration-tests/cluster_itest_util.h M src/kudu/integration-tests/tablet_copy-itest.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_copy_service.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_server-test-base.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server_test_util.cc M src/kudu/tserver/tablet_server_test_util.h 11 files changed, 249 insertions(+), 64 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/7985/3 -- To view, visit http://gerrit.cloudera.org:8080/7985 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Adar Dembo has posted comments on this change. Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. Patch Set 3: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/7998/2/.gitignore File .gitignore: Line 33: README.html > Yep. Not necessarily; I just wanted to make sure I understood the motivation for the inclusion. -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: Yes
[kudu-CR] block id: use a better hash function
Adar Dembo has posted comments on this change. Change subject: block_id: use a better hash function .. Patch Set 1: Would be great if you could synthesize a unit test that exhibits the improvement too, so that if we wanted to reevaluate the choice of hash function in the future, we wouldn't need a full cluster to do so. -- To view, visit http://gerrit.cloudera.org:8080/8001 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I30717955f962957d109a6403b55d59ab6c446a87 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Mike Percy has posted comments on this change. Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: PS3, Line 339: // Create() should not clobber. > I think the placement of this comment might be a bit confusing. Should this removed this comment http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager-stress-test.cc File src/kudu/consensus/consensus_meta_manager-stress-test.cc: PS3, Line 18: #include > nit: prefer using Done http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager.cc File src/kudu/consensus/consensus_meta_manager.cc: PS3, Line 97: if (s.ok()) > Did you consider narrowing the permissible errors here? e.g. if we hit an I Thanks for the catch. That's what I meant to do. Fixed. http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/consensus_meta_manager.h File src/kudu/consensus/consensus_meta_manager.h: PS3, Line 59: ConsensusMetadataCreateMode create_mode > What if this parameter was set to ConsensusMetadataCreateMode::FLUSH_ON_CRE Done http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/consensus/raft_consensus_quorum-test.cc File src/kudu/consensus/raft_consensus_quorum-test.cc: PS3, Line 191: scoped_refptr cmeta; > nit: drop this and leave the last parameter of the Create() call below by d Done http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/integration-tests/tombstoned_voting-itest.cc File src/kudu/integration-tests/tombstoned_voting-itest.cc: PS3, Line 18: #include > nit: consider replacing with and adding that into the list of the Done http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: PS3, Line 260: scoped_refptr cmeta; > nit: consider dropping this since it's not used in the scope and leaving th Done http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tablet/tablet_bootstrap-test.cc File src/kudu/tablet/tablet_bootstrap-test.cc: PS3, Line 146: scoped_refptr cmeta; > Nit: it seems this is not used, consider dropping it and leaving the last p Done http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tablet/tablet_replica-test.cc File src/kudu/tablet/tablet_replica-test.cc: PS3, Line 141: scoped_refptr cmeta; > Nit: consider dropping this since it's not used, leaving the last parameter Done http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: PS3, Line 601: scoped_refptr cmeta; > Nit: cmeta is not used in this scope, right? Consider dropping it. Done http://gerrit.cloudera.org:8080/#/c/7988/3/src/kudu/tserver/tablet_copy_source_session-test.cc File src/kudu/tserver/tablet_copy_source_session-test.cc: PS3, Line 157: scoped_refptr cmeta; > nit: consider dropping cmeta -- it's not used in this scope elsewhere but p Done -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7988 to look at the new patch set (#4). Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup It is possible for tombstoned replicas to legitimately not have a cmeta file as a result of crashing during a first tablet copy, or failing a tablet copy operation in an older version of Kudu. Not having a cmeta file results in those tombstoned replicas being unable to vote in Raft leader elections. We remedy this by creating a cmeta object (with an empty config) at startup time. The empty config is safe for a tombstoned replica, because the config doesn't affect a replica's ability to vote in a leader election. Additionally, if the tombstoned replica were ever to be overwritten by a tablet copy operation, that would also result in overwriting the config stored in the local cmeta with a valid Raft config. Finally, all of this assumes that the nonexistence of a cmeta file guarantees that the replica has never voted in a leader election. As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE flag, meaning that it will only be flushed to disk if the replica ever votes. The following changes had to be made to ConsensusMetadata and the ConsensusMetadataManager to support the above functionality: * Enable deferred flush on Create() by defining a flag called NO_FLUSH_ON_CREATE * Simplify the interface controlling whether a Flush() is allowed to overwrite (clobber) an existing file by encapsulating that logic in the implementation, instead of the public interface to ConsensusMetadata. When a cmeta is instantiated via ConsensusMetadata::Create(), the next Flush() is not allowed to overwrite an existing file. In every other case, Flush() is allowed to overwrite an existing file. * Made some additional method arguments optional, for convenience. The following tests have been added: * A unit test for NO_FLUSH_ON_CREATE. * A test that crashes the target of a tablet copy after writing the superblock and before writing the cmeta file. The tablet server is restarted and the replica is expected to be able to vote while tombstoned. Previously-written tests that verify ConsensusMetadata::Create() will not clobber an existing file still pass. Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b --- M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/consensus_meta_manager-test.cc M src/kudu/consensus/consensus_meta_manager.cc M src/kudu/consensus/consensus_meta_manager.h M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/tombstoned_voting-itest.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc 15 files changed, 345 insertions(+), 65 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/7988/4 -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2055[part 1]: Coalesce hole punching when deleting groups of blocks
Hao Hao has posted comments on this change. Change subject: KUDU-2055[part 1]: Coalesce hole punching when deleting groups of blocks .. Patch Set 4: The failed test TestCopyFromCrashedSource is a known flaky test (KUDU-2109). -- To view, visit http://gerrit.cloudera.org:8080/7656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy
Hello Dan Burkert, Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7966 to look at the new patch set (#6). Change subject: KUDU-2134: Defer block transaction commit to the end of a tablet copy .. KUDU-2134: Defer block transaction commit to the end of a tablet copy KUDU-2131 uncovered a regression that causes a tablet copy session to expire reliably if the copied tablet is large and the tserver already has a high number of containers. Even though the issue is mitigated by LIFO container selection, we can completely avoid it by deferring the block transaction commit to the end of tablet copy session. There are mainly two reasons we may still want to do so: 1) If DownloadWALs fails there's no reason to pay the fsync() price and commit all those blocks. 2) While DownloadWALs is running the kernel has more time to eagerly flush the blocks, so the fsync()s at the end could be cheaper. This patch moves the block transaction commit out from FetchAll() and commits all downloaded blocks before replacing the superblock. Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b --- M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h 3 files changed, 40 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/7966/6 -- To view, visit http://gerrit.cloudera.org:8080/7966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7656 to look at the new patch set (#5). Change subject: KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks .. KUDU-2055 [part 1]: Coalesce hole punching when deleting groups of blocks Similar to 'BlockCreationTransaction', this patch adds a new layer of abstraction at Block Manager to coalesce blocks deletions in a logical operation, e.g. compaction. By coalescing blocks deletions, the number of hole punching in LBM will be reduced from one per block to one per log block container. This should overall optimize the performance of operation that involves batch deletions, such as compaction and hole repunching. This patch is the first part of a serial. It only adds the new abstraction 'BlockDeletionTransaction', and doesn't yet change any deletion semantics or improve performance. Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959 --- M src/kudu/cfile/bloomfile.cc M src/kudu/cfile/bloomfile.h M src/kudu/cfile/cfile-test.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/tablet/deltafile.cc M src/kudu/tablet/deltafile.h M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/multi_column_writer.cc M src/kudu/tablet/multi_column_writer.h M src/kudu/tserver/tablet_copy_client.h 19 files changed, 157 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/56/7656/5 -- To view, visit http://gerrit.cloudera.org:8080/7656 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iecb252b3a5665d5471bb82301d0c8012a68de959 Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2124. Don't hold session lock while initializing a TabletCopySession
Adar Dembo has posted comments on this change. Change subject: KUDU-2124. Don't hold session lock while initializing a TabletCopySession .. Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/7985/3/src/kudu/integration-tests/tablet_copy-itest.cc File src/kudu/integration-tests/tablet_copy-itest.cc: Line 1643: Status s = itest::BeginTabletCopySession(ts, tablet_id, "dummy-uuid", kTimeout, &resp); Since the response isn't actually used, consider dropping it from BeginTabletCopySession. It can always be added later. Line 1674: ASSERT_LT(lat, kInjectedLatency); This seems risky; a variable amount of latency could be added to a failure from load and scheduling. http://gerrit.cloudera.org:8080/#/c/7985/3/src/kudu/tserver/tablet_copy_service-test.cc File src/kudu/tserver/tablet_copy_service-test.cc: Line 265: } If you expect some contention for a fixed amount of time, it might be useful to add a Sleep here rather than making it a tight loop to make the test is a little more CPU friendly. http://gerrit.cloudera.org:8080/#/c/7985/3/src/kudu/tserver/tablet_copy_service.cc File src/kudu/tserver/tablet_copy_service.cc: PS3, Line 185: SessionEntry* session_entry = FindOrNull(sessions_, session_id); : // Ensure that another interleaved thread has not already removed the failed session. : if (session_entry && session == session_entry->session) { : sessions_.erase(session_id); : } Can you add to the comment to explain the identity check? I imagine it's to protect against another thread not only removing this session but also replacing it with another one. Line 267: // The corresponding session is already in the process of initializing. Can you use RPC_RETURN_NOT_OK here? http://gerrit.cloudera.org:8080/#/c/7985/3/src/kudu/tserver/tablet_copy_source_session.cc File src/kudu/tserver/tablet_copy_source_session.cc: Line 73: TAG_FLAG(tablet_copy_session_inject_latency_on_init_ms, unsafe); Make it hidden. Line 74: TAG_FLAG(tablet_copy_session_inject_latency_on_init_ms, runtime); I don't see the new test changing the value of this flag at runtime; can this be omitted? PS3, Line 128: TRACE("Injecting $0ms of latency due to --tablet_copy_session_inject_latency_on_init_ms", : FLAGS_tablet_copy_session_inject_latency_on_init_ms); Is this useful to the tests that usei t? -- To view, visit http://gerrit.cloudera.org:8080/7985 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If8bd295a59ade8c89fdf1853dd64c4bceca8da91 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy
Adar Dembo has posted comments on this change. Change subject: KUDU-2134: Defer block transaction commit to the end of a tablet copy .. Patch Set 6: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/7966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy
Dan Burkert has posted comments on this change. Change subject: KUDU-2134: Defer block transaction commit to the end of a tablet copy .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/7966/5/src/kudu/tserver/tablet_copy_client-test.cc File src/kudu/tserver/tablet_copy_client-test.cc: Line 180: ASSERT_OK(client_->transaction_.CommitCreatedBlocks()); > Finish() has some other logic which changes the state of tablet copy. I was yep -- To view, visit http://gerrit.cloudera.org:8080/7966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy
Dan Burkert has posted comments on this change. Change subject: KUDU-2134: Defer block transaction commit to the end of a tablet copy .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7988 to look at the new patch set (#5). Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup It is possible for tombstoned replicas to legitimately not have a cmeta file as a result of crashing during a first tablet copy, or failing a tablet copy operation in an older version of Kudu. Not having a cmeta file results in those tombstoned replicas being unable to vote in Raft leader elections. We remedy this by creating a cmeta object (with an empty config) at startup time. The empty config is safe for a tombstoned replica, because the config doesn't affect a replica's ability to vote in a leader election. Additionally, if the tombstoned replica were ever to be overwritten by a tablet copy operation, that would also result in overwriting the config stored in the local cmeta with a valid Raft config. Finally, all of this assumes that the nonexistence of a cmeta file guarantees that the replica has never voted in a leader election. As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE flag, meaning that it will only be flushed to disk if the replica ever votes. The following changes had to be made to ConsensusMetadata and the ConsensusMetadataManager to support the above functionality: * Enable deferred flush on Create() by defining a flag called NO_FLUSH_ON_CREATE * Simplify the interface controlling whether a Flush() is allowed to overwrite (clobber) an existing file by encapsulating that logic in the implementation, instead of the public interface to ConsensusMetadata. When a cmeta is instantiated via ConsensusMetadata::Create(), the next Flush() is not allowed to overwrite an existing file. In every other case, Flush() is allowed to overwrite an existing file. * Made some additional method arguments optional, for convenience. The following tests have been added: * A unit test for NO_FLUSH_ON_CREATE. * A test that crashes the target of a tablet copy after writing the superblock and before writing the cmeta file. The tablet server is restarted and the replica is expected to be able to vote while tombstoned. Previously-written tests that verify ConsensusMetadata::Create() will not clobber an existing file still pass. Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b --- M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/consensus_meta_manager-test.cc M src/kudu/consensus/consensus_meta_manager.cc M src/kudu/consensus/consensus_meta_manager.h M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/tombstoned_voting-itest.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc 15 files changed, 345 insertions(+), 68 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/7988/5 -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [iwyu] kudu-specific mappings for std::tr1 entities
Alexey Serbin has submitted this change and it was merged. Change subject: [iwyu] kudu-specific mappings for std::tr1 entities .. [iwyu] kudu-specific mappings for std::tr1 entities Added Kudu-specific IWYU mappings for std::tr1::shared_ptr and friends from TR1 (those are used in the Kudu C++ client API). Added --max_line_length=256 IWYU option for better reporting on the reason for header files inclusion. If keeping the default setting of 80 characters, IWYU sometimes omits the information which symbols require particular header file to be included. Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Reviewed-on: http://gerrit.cloudera.org:8080/7998 Reviewed-by: Adar Dembo Tested-by: Kudu Jenkins --- M .gitignore M build-support/iwyu/iwyu-filter.awk M build-support/iwyu/iwyu.sh A build-support/iwyu/mappings/kudu.imp 4 files changed, 56 insertions(+), 18 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7998 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ia448dedcbb09ee23b30a922c7143df9036490d84 Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-test
Mike Percy has posted comments on this change. Change subject: Rename tombstoned_voting-itest to tombstoned_voting-test .. Patch Set 1: (2 comments) > tombstoned_voting-itest.cc --> tombstoned_voting-imc-itest.cc ? Done http://gerrit.cloudera.org:8080/#/c/7991/1//COMMIT_MSG Commit Message: PS1, Line 11: prefix > suffix? Done PS1, Line 12: prefix > suffix? Done -- To view, visit http://gerrit.cloudera.org:8080/7991 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-imc-itest
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7991 to look at the new patch set (#2). Change subject: Rename tombstoned_voting-itest to tombstoned_voting-imc-itest .. Rename tombstoned_voting-itest to tombstoned_voting-imc-itest tombstoned_voting-itest uses an InternalMiniCluster, and it would be useful to have tombstoned voting tests that use an ExternalMiniCluster. Rename the existing IMC-based test to make room for an EMC-based test. Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06 --- M src/kudu/integration-tests/CMakeLists.txt R src/kudu/integration-tests/tombstoned_voting-imc-itest.cc 2 files changed, 10 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/7991/2 -- To view, visit http://gerrit.cloudera.org:8080/7991 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7988 to look at the new patch set (#6). Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup It is possible for tombstoned replicas to legitimately not have a cmeta file as a result of crashing during a first tablet copy, or failing a tablet copy operation in an older version of Kudu. Not having a cmeta file results in those tombstoned replicas being unable to vote in Raft leader elections. We remedy this by creating a cmeta object (with an empty config) at startup time. The empty config is safe for a tombstoned replica, because the config doesn't affect a replica's ability to vote in a leader election. Additionally, if the tombstoned replica were ever to be overwritten by a tablet copy operation, that would also result in overwriting the config stored in the local cmeta with a valid Raft config. Finally, all of this assumes that the nonexistence of a cmeta file guarantees that the replica has never voted in a leader election. As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE flag, meaning that it will only be flushed to disk if the replica ever votes. The following changes had to be made to ConsensusMetadata and the ConsensusMetadataManager to support the above functionality: * Enable deferred flush on Create() by defining a flag called NO_FLUSH_ON_CREATE * Simplify the interface controlling whether a Flush() is allowed to overwrite (clobber) an existing file by encapsulating that logic in the implementation, instead of the public interface to ConsensusMetadata. When a cmeta is instantiated via ConsensusMetadata::Create(), the next Flush() is not allowed to overwrite an existing file. In every other case, Flush() is allowed to overwrite an existing file. * Made some additional method arguments optional, for convenience. The following tests have been added: * A unit test for NO_FLUSH_ON_CREATE. * A test that crashes the target of a tablet copy after writing the superblock and before writing the cmeta file. The tablet server is restarted and the replica is expected to be able to vote while tombstoned. Previously-written tests that verify ConsensusMetadata::Create() will not clobber an existing file still pass. Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b --- M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/consensus_meta_manager-test.cc M src/kudu/consensus/consensus_meta_manager.cc M src/kudu/consensus/consensus_meta_manager.h M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/tombstoned_voting-itest.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc 15 files changed, 345 insertions(+), 68 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/7988/6 -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-imc-itest
Alexey Serbin has posted comments on this change. Change subject: Rename tombstoned_voting-itest to tombstoned_voting-imc-itest .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7991 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced
Mike Percy has submitted this change and it was merged. Change subject: Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced .. Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced TestFailedTabletsAreReplaced manually fails the replica after only verifying that the tablet exists, with no regard for its state. This can cause the replica's bootstrap process to fail a check: F0907 00:05:46.153576 2697 tablet_replica.cc:173] Check failed: BOOTSTRAPPING == state_ (0 vs. 2) This is a test-only race where the replica successfully goes through the bootstrap process, the tablet is failed in test, and TabletReplica::Start() is called on the replica, which requires its state to be BOOTSTRAPPING. This is not an issue seen in production, as bootstrapping is normally only run if the replica is not failed, but it did result in 6/1000 failures when run in release mode with --stress_cpu_thres=32. To fix this, the replica is failed only after it is verified to be running. In doing so, the number of failures went from 6/1000 to 0/1000. Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e Reviewed-on: http://gerrit.cloudera.org:8080/7993 Tested-by: Kudu Jenkins Reviewed-by: Adar Dembo Reviewed-by: Hao Hao Reviewed-by: Mike Percy --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc 1 file changed, 16 insertions(+), 7 deletions(-) Approvals: Hao Hao: Looks good to me, but someone else must approve Mike Percy: Looks good to me, approved Adar Dembo: Looks good to me, but someone else must approve Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] Fix flakiness of ts tablet manager itest TestFailedTabletsAreReplaced
Mike Percy has posted comments on this change. Change subject: Fix flakiness of ts_tablet_manager_itest TestFailedTabletsAreReplaced .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7993 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I93b41c8196397ea5af42ed9e2aa47e967f7a520e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] Rename tombstoned voting-itest to tombstoned voting-imc-itest
Mike Percy has submitted this change and it was merged. Change subject: Rename tombstoned_voting-itest to tombstoned_voting-imc-itest .. Rename tombstoned_voting-itest to tombstoned_voting-imc-itest tombstoned_voting-itest uses an InternalMiniCluster, and it would be useful to have tombstoned voting tests that use an ExternalMiniCluster. Rename the existing IMC-based test to make room for an EMC-based test. Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06 Reviewed-on: http://gerrit.cloudera.org:8080/7991 Reviewed-by: Alexey Serbin Tested-by: Kudu Jenkins --- M src/kudu/integration-tests/CMakeLists.txt R src/kudu/integration-tests/tombstoned_voting-imc-itest.cc 2 files changed, 10 insertions(+), 10 deletions(-) Approvals: Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7991 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: Iea9fd979ca1bb61450a7b54d7b85920674aefd06 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Alexey Serbin has posted comments on this change. Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. Patch Set 6: (7 comments) I have the following question: why do we need to introduce that additional state into ConsensusMetadata object? Why not to leave the OVERWRITE/NO_OVERWRITE parameter for its Flush() method? http://gerrit.cloudera.org:8080/#/c/7988/4/src/kudu/consensus/consensus_meta-test.cc File src/kudu/consensus/consensus_meta-test.cc: PS4, Line 18: #include nit: consider returning back to . Sorry, I missed this in the first revision. http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: PS6, Line 334: /*overwrite_first_flush=*/ false Why not just to pass the NO_OVERWRITE flag to the Flush() method as it was before? I'm not a big fan of introducing some more object state in here. http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/consensus/consensus_meta_manager.cc File src/kudu/consensus/consensus_meta_manager.cc: Line 96: return Create(tablet_id, config, initial_term, create_mode); Does it make sense to pass the cmeta_out here as well? http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tablet/tablet_replica-test.cc File src/kudu/tablet/tablet_replica-test.cc: PS6, Line 86: using consensus::ConsensusMetadataCreateMode; nit: is it needed? http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tserver/tablet_copy_source_session-test.cc File src/kudu/tserver/tablet_copy_source_session-test.cc: PS6, Line 91: using consensus::ConsensusMetadataCreateMode; nit: is it needed? http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: PS6, Line 127: using consensus::ConsensusMetadataCreateMode; nit: is it needed? PS6, Line 1093: scoped_refptr cmeta; nit: it seems the 'cmeta' is not used in this scope; drop it, maybe? -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Mike Percy has posted comments on this change. Change subject: KUDU-2138 delete failed replicas in tablet report .. Patch Set 4: Code-Review+1 (3 comments) looks good, just a couple nits http://gerrit.cloudera.org:8080/#/c/7992/4/src/kudu/integration-tests/tablet_replacement-itest.cc File src/kudu/integration-tests/tablet_replacement-itest.cc: PS4, Line 182: nit: extra indentation http://gerrit.cloudera.org:8080/#/c/7992/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS4, Line 2488: Tablet Replica? PS4, Line 2492: current index is $1 how about: current committed config index is $1 -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Andrew Wong has posted comments on this change. Change subject: KUDU-2138 delete failed replicas in tablet report .. Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/7992/4/src/kudu/integration-tests/tablet_replacement-itest.cc File src/kudu/integration-tests/tablet_replacement-itest.cc: PS4, Line 182: AS > nit: extra indentation Done http://gerrit.cloudera.org:8080/#/c/7992/4/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS4, Line 2488: Replic > Replica? Done PS4, Line 2492: fig index is $1)", > how about: current committed config index is $1 Done -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: Yes
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Hello Mike Percy, Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7992 to look at the new patch set (#5). Change subject: KUDU-2138 delete failed replicas in tablet report .. KUDU-2138 delete failed replicas in tablet report When a Kudu master processes a replica's tablet report, it will send a DeleteTablet RPC if the replica is not already deleted, has a consensus state, has a lower opid index than the latest reported committed config, and is no longer in the tablet's Raft configuration. Failed tablets are handled specially in processing the report to return before any of the above checks can be made. This can lead to failed tablets lingering when they should actually be deleted. This patch fixes this by performing these checks and sending the delete regardless of whether or not the tablet is failed (other than checking the opid index, since failed tablets do not report an opid index). A test is added to tablet_replacement-itest to fail a replica, evict but not tombstone it, report it to the master, and ensure it is properly deleted. Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c --- M src/kudu/integration-tests/tablet_replacement-itest.cc M src/kudu/master/catalog_manager.cc 2 files changed, 102 insertions(+), 23 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/7992/5 -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Mike Percy has submitted this change and it was merged. Change subject: KUDU-2138 delete failed replicas in tablet report .. KUDU-2138 delete failed replicas in tablet report When a Kudu master processes a replica's tablet report, it will send a DeleteTablet RPC if the replica is not already deleted, has a consensus state, has a lower opid index than the latest reported committed config, and is no longer in the tablet's Raft configuration. Failed tablets are handled specially in processing the report to return before any of the above checks can be made. This can lead to failed tablets lingering when they should actually be deleted. This patch fixes this by performing these checks and sending the delete regardless of whether or not the tablet is failed (other than checking the opid index, since failed tablets do not report an opid index). A test is added to tablet_replacement-itest to fail a replica, evict but not tombstone it, report it to the master, and ensure it is properly deleted. Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Reviewed-on: http://gerrit.cloudera.org:8080/7992 Tested-by: Kudu Jenkins Reviewed-by: Mike Percy --- M src/kudu/integration-tests/tablet_replacement-itest.cc M src/kudu/master/catalog_manager.cc 2 files changed, 102 insertions(+), 23 deletions(-) Approvals: Mike Percy: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy
[kudu-CR] KUDU-2138 delete failed replicas in tablet report
Mike Percy has posted comments on this change. Change subject: KUDU-2138 delete failed replicas in tablet report .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7992 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I34abd2cf4d00098a28dd7645053ccdc8341df03c Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-HasComments: No
[kudu-CR] block id: use a better hash function
Todd Lipcon has posted comments on this change. Change subject: block_id: use a better hash function .. Patch Set 1: I tried making a unit test and weirdly in my unit test environment, the new hash was actually worse than the old one. I also tried reverting this but leaving my other optimizations, and it turns out in the presence of the other fixes around refcounting, etc, and using sparsepp instead of sparsehash, this doesn't make a difference. So, I'll abandon for now. -- To view, visit http://gerrit.cloudera.org:8080/8001 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I30717955f962957d109a6403b55d59ab6c446a87 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy
Mike Percy has posted comments on this change. Change subject: KUDU-2134: Defer block transaction commit to the end of a tablet copy .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/7966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: No
[kudu-CR] KUDU-2134: Defer block transaction commit to the end of a tablet copy
Mike Percy has submitted this change and it was merged. Change subject: KUDU-2134: Defer block transaction commit to the end of a tablet copy .. KUDU-2134: Defer block transaction commit to the end of a tablet copy KUDU-2131 uncovered a regression that causes a tablet copy session to expire reliably if the copied tablet is large and the tserver already has a high number of containers. Even though the issue is mitigated by LIFO container selection, we can completely avoid it by deferring the block transaction commit to the end of tablet copy session. There are mainly two reasons we may still want to do so: 1) If DownloadWALs fails there's no reason to pay the fsync() price and commit all those blocks. 2) While DownloadWALs is running the kernel has more time to eagerly flush the blocks, so the fsync()s at the end could be cheaper. This patch moves the block transaction commit out from FetchAll() and commits all downloaded blocks before replacing the superblock. Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b Reviewed-on: http://gerrit.cloudera.org:8080/7966 Reviewed-by: Adar Dembo Reviewed-by: Dan Burkert Tested-by: Kudu Jenkins Reviewed-by: Mike Percy --- M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_client.h 3 files changed, 40 insertions(+), 36 deletions(-) Approvals: Dan Burkert: Looks good to me, approved Mike Percy: Looks good to me, approved Adar Dembo: Looks good to me, but someone else must approve Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7966 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I4feabc08c2199aad8d08be56f09ac06924345f2b Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Mike Percy has posted comments on this change. Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. Patch Set 6: (6 comments) > (7 comments) > > I have the following question: why do we need to introduce that > additional state into ConsensusMetadata object? Why not to leave > the OVERWRITE/NO_OVERWRITE parameter for its Flush() method? After discussing this with you offline, I agree that we can simplify the implementation and avoid this additional state. I'll back out this portion of the patch. http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: PS6, Line 334: /*overwrite_first_flush=*/ false > Why not just to pass the NO_OVERWRITE flag to the Flush() method as it was The semantics I want to maintain are that we don't overwrite on the first Flush() call after create, but we overwrite at any other time. http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/consensus/consensus_meta_manager.cc File src/kudu/consensus/consensus_meta_manager.cc: Line 96: return Create(tablet_id, config, initial_term, create_mode); > Does it make sense to pass the cmeta_out here as well? Fixed, added a unit test too. http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tablet/tablet_replica-test.cc File src/kudu/tablet/tablet_replica-test.cc: PS6, Line 86: using consensus::ConsensusMetadataCreateMode; > nit: is it needed? Done http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tserver/tablet_copy_source_session-test.cc File src/kudu/tserver/tablet_copy_source_session-test.cc: PS6, Line 91: using consensus::ConsensusMetadataCreateMode; > nit: is it needed? Done http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: PS6, Line 127: using consensus::ConsensusMetadataCreateMode; > nit: is it needed? This one is needed :) PS6, Line 1093: scoped_refptr cmeta; > nit: it seems the 'cmeta' is not used in this scope; drop it, maybe? Done -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/7988 to look at the new patch set (#7). Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup It is possible for tombstoned replicas to legitimately not have a cmeta file as a result of crashing during a first tablet copy, or failing a tablet copy operation in an older version of Kudu. Not having a cmeta file results in those tombstoned replicas being unable to vote in Raft leader elections. We remedy this by creating a cmeta object (with an empty config) at startup time. The empty config is safe for a tombstoned replica, because the config doesn't affect a replica's ability to vote in a leader election. Additionally, if the tombstoned replica were ever to be overwritten by a tablet copy operation, that would also result in overwriting the config stored in the local cmeta with a valid Raft config. Finally, all of this assumes that the nonexistence of a cmeta file guarantees that the replica has never voted in a leader election. As an optimization, the cmeta is created with the NO_FLUSH_ON_CREATE flag, meaning that it will only be flushed to disk if the replica ever votes. The following changes had to be made to ConsensusMetadata and the ConsensusMetadataManager to support the above functionality: * Enable deferred flush on Create() by defining a flag called NO_FLUSH_ON_CREATE * Made some additional method arguments optional, for convenience. The following tests have been added: * A unit test for NO_FLUSH_ON_CREATE. * A test that crashes the target of a tablet copy after writing the superblock and before writing the cmeta file. The tablet server is restarted and the replica is expected to be able to vote while tombstoned. Previously-written tests that verify ConsensusMetadata::Create() will not clobber an existing file still pass, and an additional test was addedadded for unflushed cmeta instances. Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b --- M src/kudu/consensus/consensus_meta-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/consensus_meta.h M src/kudu/consensus/consensus_meta_manager-test.cc M src/kudu/consensus/consensus_meta_manager.cc M src/kudu/consensus/consensus_meta_manager.h M src/kudu/consensus/raft_consensus_quorum-test.cc M src/kudu/integration-tests/CMakeLists.txt A src/kudu/integration-tests/tombstoned_voting-itest.cc M src/kudu/master/sys_catalog.cc M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_replica-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_source_session-test.cc M src/kudu/tserver/ts_tablet_manager.cc 15 files changed, 352 insertions(+), 62 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/7988/7 -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2137: protect against concurrent schema version change and tablet drop
Alexey Serbin has posted comments on this change. Change subject: KUDU-2137: protect against concurrent schema version change and tablet drop .. Patch Set 1: Just FYI: to me, the test reliably fails with 1/512 ratio if built in debug mode: http://dist-test.cloudera.org//job?job_id=aserbin.1504831083.9508 http://dist-test.cloudera.org//job?job_id=aserbin.1504831720.12459 -- To view, visit http://gerrit.cloudera.org:8080/7996 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I371fc310a97ae94ec2ebf04405db99c5f2937e1a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] KUDU-2137: protect against concurrent schema version change and tablet drop
Alexey Serbin has posted comments on this change. Change subject: KUDU-2137: protect against concurrent schema version change and tablet drop .. Patch Set 1: > Just FYI: to me, the test reliably fails with 1/512 ratio if built > in debug mode: > > http://dist-test.cloudera.org//job?job_id=aserbin.1504831083.9508 > http://dist-test.cloudera.org//job?job_id=aserbin.1504831720.12459 I ran that with --stress_cpu_threads=8 -- To view, visit http://gerrit.cloudera.org:8080/7996 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I371fc310a97ae94ec2ebf04405db99c5f2937e1a Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] web ui: fix "percentage consumed" calculation
Todd Lipcon has submitted this change and it was merged. Change subject: web ui: fix "percentage consumed" calculation .. web ui: fix "percentage consumed" calculation There was a misplaced cast, so the division of consumption by limit was using an integer rather than floating point calculation. This results in the "percentage consumed" always showing 0. Change-Id: I07d5b7d5f44548120a9b31bfef43e23051e27d8e Reviewed-on: http://gerrit.cloudera.org:8080/7987 Tested-by: Kudu Jenkins Reviewed-by: Dan Burkert --- M src/kudu/server/default-path-handlers.cc 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Dan Burkert: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/7987 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I07d5b7d5f44548120a9b31bfef43e23051e27d8e Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [tests] fix flakes in delete table-itest
Mike Percy has posted comments on this change. Change subject: [tests] fix flakes in delete_table-itest .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/7972/3/src/kudu/integration-tests/delete_table-itest.cc File src/kudu/integration-tests/delete_table-itest.cc: PS3, Line 1216: : : Instead of adding the extra functions, could we do this in less by just wrapping the logic of finding the leader and changing the config in an ASSERT_EVENTUALLY()? i.e.: // Loop in case the leader changes between adding and removing a replica. for (int i : {0, 1}) { ASSERT_EVENTUALLY([&] { // Find leader. TServerDetails* leader = nullptr; ASSERT_OK(FindTabletLeader(ts_map_, tablet_id, kTimeout, &leader)); ASSERT_OK(WaitForOpFromCurrentTerm(leader, tablet_id, COMMITTED_OPID, kTimeout)); // Do the config changes and wait for the master to delete the old node. if (i == 0) { ASSERT_OK(AddServer(leader, tablet_id, to_add, RaftPeerPB::VOTER, boost::none, kTimeout)); } else { ASSERT_OK(RemoveServer(leader, tablet_id, to_remove, boost::none, kTimeout)); } }); } -- To view, visit http://gerrit.cloudera.org:8080/7972 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7dda40ae1054becaf25963a6d301ecaed5a926f9 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
[kudu-CR] log block manager: switch from google::sparse hash map to sparsepp
Hello Dan Burkert, Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8007 to review the following change. Change subject: log_block_manager: switch from google::sparse_hash_map to sparsepp .. log_block_manager: switch from google::sparse_hash_map to sparsepp sparsepp is updated for C++11 so it enables move semantics for the map elements. Since the block map uses ref-counted values, being able to move them is a big win. It also claims to be generally faster even aside from the ability to support moves. This improved startup time 7-8x on a real host with ~11M blocks: Before: I0907 17:23:50.748055 12507 fs_manager.cc:335] Time spent opening block manager: real 108.910s user 0.000s sys 0.001s After: I0907 17:20:42.277474 10021 fs_manager.cc:335] Time spent opening block manager: real 14.348s user 0.000s sys 0.001s The LBM startup benchmark (1M blocks) improved less substantially but still noticeably: Before: I0907 17:16:54.899818 20839 log_block_manager-test.cc:799] Time spent reopening block manager: real 2.612s user 0.035s sys 0.002s I0907 17:16:57.498205 20839 log_block_manager-test.cc:799] Time spent reopening block manager: real 2.598s user 0.039s sys 0.001s I0907 17:17:00.100244 20839 log_block_manager-test.cc:799] Time spent reopening block manager: real 2.602s user 0.042s sys 0.000s I0907 17:17:02.686638 20839 log_block_manager-test.cc:799] Time spent reopening block manager: real 2.586s user 0.042s sys 0.000s I0907 17:17:05.284050 20839 log_block_manager-test.cc:799] Time spent reopening block manager: real 2.597s user 0.041s sys 0.001s I0907 17:17:07.884395 20839 log_block_manager-test.cc:799] Time spent reopening block manager: real 2.600s user 0.039s sys 0.001s I0907 17:17:10.490550 20839 log_block_manager-test.cc:799] Time spent reopening block manager: real 2.606s user 0.040s sys 0.001s I0907 17:17:13.070114 20839 log_block_manager-test.cc:799] Time spent reopening block manager: real 2.580s user 0.039s sys 0.000s I0907 17:17:15.667062 20839 log_block_manager-test.cc:799] Time spent reopening block manager: real 2.597s user 0.040s sys 0.001s I0907 17:17:18.258447 20839 log_block_manager-test.cc:799] Time spent reopening block manager: real 2.591s user 0.042s sys 0.000s After: I0907 17:15:50.645310 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.570s user 0.034s sys 0.001s I0907 17:15:52.195543 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.550s user 0.037s sys 0.001s I0907 17:15:53.755209 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.560s user 0.037s sys 0.001s I0907 17:15:55.263762 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.509s user 0.038s sys 0.001s I0907 17:15:56.818748 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.555s user 0.037s sys 0.001s I0907 17:15:58.379680 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.561s user 0.036s sys 0.001s I0907 17:15:59.913751 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.534s user 0.038s sys 0.000s I0907 17:16:01.461668 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.548s user 0.037s sys 0.001s I0907 17:16:03.020823 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.559s user 0.037s sys 0.001s I0907 17:16:04.549747 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.529s user 0.035s sys 0.001s Change-Id: I7397f9cd418782caecf8b2dae2c7bfe2c0e6215c --- M src/kudu/fs/log_block_manager.h M thirdparty/build-definitions.sh M thirdparty/build-thirdparty.sh M thirdparty/download-thirdparty.sh M thirdparty/vars.sh 5 files changed, 35 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/8007/1 -- To view, visit http://gerrit.cloudera.org:8080/8007 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I7397f9cd418782caecf8b2dae2c7bfe2c0e6215c Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert
[kudu-CR] Avoid a few allocations while reading PBC files
Hello Dan Burkert, Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8009 to review the following change. Change subject: Avoid a few allocations while reading PBC files .. Avoid a few allocations while reading PBC files This reduces extra short-lived allocations while reading PBC files: - pb_util now uses faststring buffers instead of heap-allocated arrays. For short reads (<32 bytes) these are stack-allocated. Since PBC files use length prefixes for records, this avoids a heap allocation per record. - env_posix used to use a temporary vector for every read. Now it uses C-style pointer-and-length for the array to avoid the allocation. This sped up startup on a real host with 11M blocks a few more percent: Before: I0907 17:27:13.600186 14550 fs_manager.cc:335] Time spent opening block manager: real 13.401s user 0.000s sys 0.002s After: I0907 17:43:46.364558 21480 fs_manager.cc:335] Time spent opening block manager: real 12.377s user 0.000s sys 0.001s This also sped up the LBM startup time benchmark a little bit: Before: I0907 17:07:40.230087 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.495s user 0.032s sys 0.001s I0907 17:07:41.642105 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.412s user 0.044s sys 0.001s I0907 17:07:43.082852 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.441s user 0.040s sys 0.001s I0907 17:07:44.512603 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.430s user 0.043s sys 0.000s I0907 17:07:45.956848 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.444s user 0.041s sys 0.000s I0907 17:07:47.386735 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.430s user 0.037s sys 0.002s I0907 17:07:48.787317 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.401s user 0.041s sys 0.002s I0907 17:07:50.244407 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.457s user 0.038s sys 0.001s I0907 17:07:51.682209 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.438s user 0.037s sys 0.001s I0907 17:07:53.116209 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.434s user 0.037s sys 0.000s After: I0907 17:40:07.984983 10204 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.429s user 0.032s sys 0.001s I0907 17:40:09.359199 10204 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.374s user 0.037s sys 0.000s I0907 17:40:10.767251 10204 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.408s user 0.038s sys 0.001s I0907 17:40:12.160768 10204 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.394s user 0.037s sys 0.001s I0907 17:40:13.539149 10204 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.378s user 0.035s sys 0.001s I0907 17:40:14.932631 10204 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.393s user 0.037s sys 0.000s I0907 17:40:16.316269 10204 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.384s user 0.037s sys 0.000s I0907 17:40:17.714495 10204 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.398s user 0.038s sys 0.000s I0907 17:40:19.077134 10204 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.363s user 0.036s sys 0.001s I0907 17:40:20.487521 10204 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.410s user 0.037s sys 0.001s Change-Id: I228f26416b750c5a30ec6cc0763257c7d8b8d56f --- M src/kudu/util/env_posix.cc M src/kudu/util/pb_util.cc 2 files changed, 17 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/09/8009/1 -- To view, visit http://gerrit.cloudera.org:8080/8009 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I228f26416b750c5a30ec6cc0763257c7d8b8d56f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert
[kudu-CR] block id: use a better hash function
Todd Lipcon has abandoned this change. Change subject: block_id: use a better hash function .. Abandoned -- To view, visit http://gerrit.cloudera.org:8080/8001 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: abandon Gerrit-Change-Id: I30717955f962957d109a6403b55d59ab6c446a87 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] WIP: pb util: avoid repeated stat() calls reading files
Hello Dan Burkert, Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8010 to review the following change. Change subject: WIP: pb_util: avoid repeated stat() calls reading files .. WIP: pb_util: avoid repeated stat() calls reading files This reduces the number of fstat syscalls while loading a host with 11M blocks from 29.3M to 147K. Change-Id: I27371800604bcb20bafae7946d3b3e84af094598 --- M src/kudu/util/pb_util.cc M src/kudu/util/pb_util.h 2 files changed, 29 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/10/8010/1 -- To view, visit http://gerrit.cloudera.org:8080/8010 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I27371800604bcb20bafae7946d3b3e84af094598 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert
[kudu-CR] log block manager: use move semantics to fill in the block map
Hello Dan Burkert, Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8008 to review the following change. Change subject: log_block_manager: use move semantics to fill in the block map .. log_block_manager: use move semantics to fill in the block map This avoids an extra ref-count increment/decrement while loading the block map during startup. This substantially improves startup time. This shaved off another few percent from LBM startup time. On a real host with 11M blocks: Before: I0907 17:20:42.277474 10021 fs_manager.cc:335] Time spent opening block manager: real 14.348s user 0.000s sys 0.001s After: I0907 17:27:13.600186 14550 fs_manager.cc:335] Time spent opening block manager: real 13.401s user 0.000s sys 0.002s According to the benchmark: Before: I0907 17:15:50.645310 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.570s user 0.034s sys 0.001s I0907 17:15:52.195543 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.550s user 0.037s sys 0.001s I0907 17:15:53.755209 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.560s user 0.037s sys 0.001s I0907 17:15:55.263762 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.509s user 0.038s sys 0.001s I0907 17:15:56.818748 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.555s user 0.037s sys 0.001s I0907 17:15:58.379680 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.561s user 0.036s sys 0.001s I0907 17:15:59.913751 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.534s user 0.038s sys 0.000s I0907 17:16:01.461668 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.548s user 0.037s sys 0.001s I0907 17:16:03.020823 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.559s user 0.037s sys 0.001s I0907 17:16:04.549747 20302 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.529s user 0.035s sys 0.001s After: I0907 17:07:40.230087 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.495s user 0.032s sys 0.001s I0907 17:07:41.642105 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.412s user 0.044s sys 0.001s I0907 17:07:43.082852 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.441s user 0.040s sys 0.001s I0907 17:07:44.512603 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.430s user 0.043s sys 0.000s I0907 17:07:45.956848 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.444s user 0.041s sys 0.000s I0907 17:07:47.386735 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.430s user 0.037s sys 0.002s I0907 17:07:48.787317 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.401s user 0.041s sys 0.002s I0907 17:07:50.244407 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.457s user 0.038s sys 0.001s I0907 17:07:51.682209 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.438s user 0.037s sys 0.001s I0907 17:07:53.116209 9782 log_block_manager-test.cc:799] Time spent reopening block manager: real 1.434s user 0.037s sys 0.000s Change-Id: I8fe65087585e118f91320ec39f537b5f3ad6552f --- M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h 2 files changed, 12 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/08/8008/1 -- To view, visit http://gerrit.cloudera.org:8080/8008 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I8fe65087585e118f91320ec39f537b5f3ad6552f Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert
[kudu-CR] Add a simple benchmark to create 1M blocks and reopen LBM
Hello Dan Burkert, Adar Dembo, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/8006 to review the following change. Change subject: Add a simple benchmark to create 1M blocks and reopen LBM .. Add a simple benchmark to create 1M blocks and reopen LBM Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878 --- M src/kudu/fs/block_id.h M src/kudu/fs/log_block_manager-test.cc 2 files changed, 27 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/06/8006/1 -- To view, visit http://gerrit.cloudera.org:8080/8006 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5cc3547db7d8389c4e89ff9d4d3043b5f2fbe878 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert
[kudu-CR] ref counted: fix move constructors to actually move
Hello Dan Burkert, Adar Dembo, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8002 to look at the new patch set (#2). Change subject: ref_counted: fix move constructors to actually move .. ref_counted: fix move constructors to actually move Ever since the move constructors for scoped_refptr were first implemented, they didn't properly move. That is to say, they caused an unnecessary AddRef/Release pair. The issue is that they were passing through an rvalue-reference parameter without properly using std::move(). See [1] for an explanation of this subtlety. When combined with some other in-flight patches, this resulted in a substantial improvement in LBM startup time, since a hashmap containing ref-counted elements could relocate them without unnecessary reference count increments and decrements. It's likely it will also improve performance elsewhere throughout Kudu. This patch also pulls in an extra move constructor from the Chromium code base which they claim helps Clang generate better warnings. I didn't verify that, but figured I'd copy it while I was looking at the code. [1] https://stackoverflow.com/questions/14486367/why-do-you-use-stdmove-when-you-have-in-c11 Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6 --- M src/kudu/gutil/ref_counted.h 1 file changed, 12 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/02/8002/2 -- To view, visit http://gerrit.cloudera.org:8080/8002 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6b6b6753ab16221e065900eba5a7c178975d38a6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-1755 Part 2: Improve tablet on-disk size metric
Mike Percy has posted comments on this change. Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric .. Patch Set 15: (2 comments) http://gerrit.cloudera.org:8080/#/c/6968/13/src/kudu/consensus/consensus_meta.h File src/kudu/consensus/consensus_meta.h: PS13, Line 244: d to > Thanks. Just FYI, here is some more fun facts about that: https://news.yco That is a really cool post from Joshua Bloch. Anyway, we have to decide whether we prefer to have one fewer bit available or have 0 - 1 == INT_MAX. The former seems better to me. :) http://gerrit.cloudera.org:8080/#/c/6968/15/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: PS15, Line 736: base_data_->OnDiskDataSize() I believe this should be OnDiskDataSize() -- To view, visit http://gerrit.cloudera.org:8080/6968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia83f8aae0a544abbd19f5a34520b4828b89b6315 Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Will Berkeley Gerrit-HasComments: Yes
[kudu-CR] KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup
Alexey Serbin has posted comments on this change. Change subject: KUDU-2123. Auto-vivify cmeta on tombstoned replicas if doesn't exist at startup .. Patch Set 7: Code-Review+2 (3 comments) > (6 comments) > > > (7 comments) > > > > I have the following question: why do we need to introduce that > > additional state into ConsensusMetadata object? Why not to leave > > the OVERWRITE/NO_OVERWRITE parameter for its Flush() method? > > After discussing this with you offline, I agree that we can > simplify the implementation and avoid this additional state. I'll > back out this portion of the patch. Thank you for addressing my concerns! As for now, LGTM, just a couple of nits. Feel free to ignore, though. http://gerrit.cloudera.org:8080/#/c/7988/7//COMMIT_MSG Commit Message: Line 40: nit: do you want to mention new tests for CreateOrLoad()? http://gerrit.cloudera.org:8080/#/c/7988/7/src/kudu/consensus/consensus_meta_manager-test.cc File src/kudu/consensus/consensus_meta_manager-test.cc: PS7, Line 138: { nit: this additional scope might be removed, I think. http://gerrit.cloudera.org:8080/#/c/7988/6/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: PS6, Line 127: using consensus::ConsensusMetadataCreateMode; > This one is needed :) oh, now I see -- that's for LoadOrCreate() call; it seems I didn't scroll down enough to see that :) -- To view, visit http://gerrit.cloudera.org:8080/7988 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8ff6255b1fcbb12417b82853bcde9b239291492b Gerrit-PatchSet: 7 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: Yes