[kudu-CR] KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16537 ) Change subject: KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior .. KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior This patch adds more consistency into the TxnStatusManager's behavior during the time interval between the following events: * The state of the leader replica of a transaction status tablet changes from BOOTSTRAPPING to RUNNING (see TabletReplica::Start() for details). * TxnStatusManager's runtime structures are populated with the information loaded from the tablet (i.e. TxnStatusManager::LoadFromTablet() successfully completes). Before this patch, relevant methods of the TxnStatusManager class could behave inconsistently within the mentioned interval. With this patch, those methods return Status::ServiceUnavailable() and do not exhibit any inconsistent behavior. This patch also adds a new test scenario to cover the new behavior. This is a follow-up to efd8c4f165460b7fa337b8ebd1856b10bc274311. Change-Id: Ia95f821d87145aff6587c017a425e205bdcb8b2b Reviewed-on: http://gerrit.cloudera.org:8080/16537 Tested-by: Kudu Jenkins Reviewed-by: Alexey Serbin --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/transactions/txn_status_manager-test.cc M src/kudu/transactions/txn_status_manager.cc M src/kudu/transactions/txn_status_manager.h 4 files changed, 139 insertions(+), 12 deletions(-) Approvals: Kudu Jenkins: Verified Alexey Serbin: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16537 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ia95f821d87145aff6587c017a425e205bdcb8b2b Gerrit-Change-Number: 16537 Gerrit-PatchSet: 7 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16537 ) Change subject: KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior .. Patch Set 6: Code-Review+2 Carrying over Hao's +2 from PS4. -- To view, visit http://gerrit.cloudera.org:8080/16537 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia95f821d87145aff6587c017a425e205bdcb8b2b Gerrit-Change-Number: 16537 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 07 Oct 2020 05:06:30 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p11: persist txn metadata in superblock
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16492 ) Change subject: KUDU-2612 p11: persist txn metadata in superblock .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f32808aef10484f4e0ad3942bb005f61fbdb34a Gerrit-Change-Number: 16492 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 07 Oct 2020 04:54:18 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p10: have timestamp assignment account for commit timestamps
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16470 ) Change subject: KUDU-2612 p10: have timestamp assignment account for commit timestamps .. Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16470 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0412fd0cf778d96f3fe6b14624d8d06942f40e72 Gerrit-Change-Number: 16470 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 07 Oct 2020 04:54:00 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16537 to look at the new patch set (#6). Change subject: KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior .. KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior This patch adds more consistency into the TxnStatusManager's behavior during the time interval between the following events: * The state of the leader replica of a transaction status tablet changes from BOOTSTRAPPING to RUNNING (see TabletReplica::Start() for details). * TxnStatusManager's runtime structures are populated with the information loaded from the tablet (i.e. TxnStatusManager::LoadFromTablet() successfully completes). Before this patch, relevant methods of the TxnStatusManager class could behave inconsistently within the mentioned interval. With this patch, those methods return Status::ServiceUnavailable() and do not exhibit any inconsistent behavior. This patch also adds a new test scenario to cover the new behavior. This is a follow-up to efd8c4f165460b7fa337b8ebd1856b10bc274311. Change-Id: Ia95f821d87145aff6587c017a425e205bdcb8b2b --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/transactions/txn_status_manager-test.cc M src/kudu/transactions/txn_status_manager.cc M src/kudu/transactions/txn_status_manager.h 4 files changed, 139 insertions(+), 12 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/16537/6 -- To view, visit http://gerrit.cloudera.org:8080/16537 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia95f821d87145aff6587c017a425e205bdcb8b2b Gerrit-Change-Number: 16537 Gerrit-PatchSet: 6 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16388 to look at the new patch set (#10). Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled This patch enhances the HMS plugin to check if synchronization is enabled on the Kudu cluster backing the table. This can simplify enabling HMS synchronization because the plugin jar can be added to the HMS classpath unconditionally. This also helps support cases where multiple Kudu clusters use the same HMS and one is synchronized and the other isn’t. This patch introduces a new environment property, `KUDU_HMS_SYNC_ENABLED`, to the plugin which allows for faster tests that do not need to setup a Kudu cluster. It could also be useful to disable this new functionality as a workaround if issues are seen. Additonally, this patch adjusts the mini HMS to add security properties configuration. This is required now that the Kudu client runs in the plugin and communicates with the cluster. See these commits for context: - https://github.com/apache/kudu/commit/0ee93e31a1febb987c72e7392a69b2584e6f38ed - https://github.com/apache/kudu/commit/3343144fefaad5a30e95e21297c64c78e308fa1f I also manually verified the following behavior on a real cluster: - `KUDU_HMS_SYNC_ENABLED` environment variable - CREATE/ALTER/DROP on non-sync cluster works - - With plugin jar used by the HMS - CREATE/ALTER/DROP on sync cluster fails with clear message - Authn via Kerberos Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 --- M java/kudu-hive/build.gradle M java/kudu-hive/src/main/java/org/apache/kudu/hive/metastore/KuduMetastorePlugin.java M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java M src/kudu/hms/hms_catalog-test.cc M src/kudu/hms/hms_client-test.cc M src/kudu/hms/mini_hms.cc M src/kudu/hms/mini_hms.h 7 files changed, 239 insertions(+), 43 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/88/16388/10 -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16388 ) Change subject: KUDU-3187: Enhance the HMS plugin to check if synchronization is enabled .. Patch Set 8: (2 comments) http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16388/8//COMMIT_MSG@25 PS8, Line 25: the Kudu client runs in the plugin and communicates : with the cluster > I'm not sure how the HMS manages its credentials, but it'd be good to at le I hit some issues writing an automated test that validates renewal is handled by the HMS, but I did manually validate this in a real environment with a short ticket and renew lifetime. I would like to commit this patch and work on adding a test later to unblock work. http://gerrit.cloudera.org:8080/#/c/16388/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java File java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java: http://gerrit.cloudera.org:8080/#/c/16388/9/java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java@a52 PS9, Line 52: > Missed this before. Why don't we want to retry these tests anymore? Should Ah, right. I was expecting to change this and forgot to add it back. Good catch. -- To view, visit http://gerrit.cloudera.org:8080/16388 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib3588d72af1bb499202b47fca50a08876e13ea37 Gerrit-Change-Number: 16388 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Greg Solovyev Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 06 Oct 2020 23:36:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior
Hello Kudu Jenkins, Andrew Wong, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16537 to look at the new patch set (#5). Change subject: KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior .. KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior This patch adds more consistency into the TxnStatusManager's behavior during the time interval between the following events: * The state of the leader replica of a transaction status tablet changes from BOOTSTRAPPING to RUNNING (see TabletReplica::Start() for details). * TxnStatusManager's runtime structures are populated with the information loaded from the tablet (i.e. TxnStatusManager::LoadFromTablet() successfully completes). Before this patch, relevant methods of the TxnStatusManager class could behave inconsistently within the mentioned interval. With this patch, those methods return Status::ServiceUnavailable() and do not exhibit any inconsistent behavior. This patch also adds a new test scenario to cover the new behavior. This is a follow-up to efd8c4f165460b7fa337b8ebd1856b10bc274311. Change-Id: Ia95f821d87145aff6587c017a425e205bdcb8b2b --- M src/kudu/integration-tests/ts_tablet_manager-itest.cc M src/kudu/transactions/txn_status_manager-test.cc M src/kudu/transactions/txn_status_manager.cc M src/kudu/transactions/txn_status_manager.h 4 files changed, 134 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/37/16537/5 -- To view, visit http://gerrit.cloudera.org:8080/16537 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia95f821d87145aff6587c017a425e205bdcb8b2b Gerrit-Change-Number: 16537 Gerrit-PatchSet: 5 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16537 ) Change subject: KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior .. Patch Set 4: > LGTM, although the approach of ensure the tablet metadata is loaded > may change with the TxnStatusManager leadership calls only patch I > am working on. Thank you for the review! Yes, I guess this should change, and I hope there should not be too many code-wise conflicts. -- To view, visit http://gerrit.cloudera.org:8080/16537 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia95f821d87145aff6587c017a425e205bdcb8b2b Gerrit-Change-Number: 16537 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 06 Oct 2020 19:57:47 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16537 ) Change subject: KUDU-2612 p2 (c): small fix on TxnStatusManager's behavior .. Patch Set 4: Code-Review+2 LGTM, although the approach of ensure the tablet metadata is loaded may change with the TxnStatusManager leadership calls only patch I am working on. -- To view, visit http://gerrit.cloudera.org:8080/16537 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia95f821d87145aff6587c017a425e205bdcb8b2b Gerrit-Change-Number: 16537 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 06 Oct 2020 19:45:53 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p11: persist txn metadata in superblock
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16492 ) Change subject: KUDU-2612 p11: persist txn metadata in superblock .. Patch Set 11: Verified+1 The failed test is unrelated to this patch: TabletCopyClientTest.TestLifeCycle -- To view, visit http://gerrit.cloudera.org:8080/16492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f32808aef10484f4e0ad3942bb005f61fbdb34a Gerrit-Change-Number: 16492 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 06 Oct 2020 19:22:13 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p11: persist txn metadata in superblock
Andrew Wong has removed a vote on this change. Change subject: KUDU-2612 p11: persist txn metadata in superblock .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/16492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I2f32808aef10484f4e0ad3942bb005f61fbdb34a Gerrit-Change-Number: 16492 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 p11: persist txn metadata in superblock
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16492 to look at the new patch set (#11). Change subject: KUDU-2612 p11: persist txn metadata in superblock .. KUDU-2612 p11: persist txn metadata in superblock Currently, transaction metadata is entirely stored in the WALs; once a transaction is begun, it anchors WALs indefinitely. This is untenable, as any tablet that participates in a transaction would never be able to GC its WALs. This patch addresses this by storing transaction metadata in the tablet superblock for the following participant ops: - BEGIN_TXN: an empty record is added to the superblock for the given transaction ID. In the future, this can be used to store the owner of the transaction. - FINALIZE_COMMIT: the commit timestamp is added to the superblock for the given transaction ID. The commit timestamp of a transaction will be used when scanning over mutations applied to a tablet instead of the apply timestamp. - ABORT_TXN: an abort bit is added to the superblock for the given transaction ID. Upon applying each of these ops, the op's OpId is anchored in the WALs until the tablet metadata is successfully flushed, ensuring that if we don't flush the tablet metadata before restarting, we can rebuild any ops that whose mutations had not been persisted by replaying the WALs. What about BEGIN_COMMIT ops? While these ops will still need to be anchored, BEGIN_COMMIT ops don't need to persist any long-term information. As such, their anchoring is slightly different -- rather than mutating the tablet metadata and unanchoring on a metadata flush, BEGIN_COMMIT ops will update the in-memory state for the in-flight Txn, and unanchor once the corresponding FINALIZE_COMMIT or ABORT_TXN begins applying. On bootstrap, the following rules are applied: - If we've persisted terminal information (i.e. abort or commit) about a transaction ID, we can skip replaying any participant op for that transaction. - If we otherwise have a persisted record for the transaction ID, we should start an open transaction and replay all participant ops for that transaction. - If we have no record of a transaction persisted, we must replay all participant ops for the given transaction. While storing things in the superblock is not ideal, there are further improvements that can be made to reduce its size, e.g. after finalizing a transaction, the transactions mutations may be merged in with the rest of the tablet; once all mutations of a given transaction have been merged, the transaction metadata may be removed from the metadata. Change-Id: I2f32808aef10484f4e0ad3942bb005f61fbdb34a --- M src/kudu/integration-tests/txn_participant-itest.cc M src/kudu/tablet/metadata.proto M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/ops/participant_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_metadata-test.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tablet/tablet_metadata.h M src/kudu/tablet/txn_participant-test.cc M src/kudu/tablet/txn_participant.cc M src/kudu/tablet/txn_participant.h 13 files changed, 739 insertions(+), 133 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/16492/11 -- To view, visit http://gerrit.cloudera.org:8080/16492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f32808aef10484f4e0ad3942bb005f61fbdb34a Gerrit-Change-Number: 16492 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 ) Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates .. Patch Set 4: Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG@16 PS2, Line 16: Before this patch, CatalogManager returned Status::InvalidArgument() : for exact duplicates and otherwise overlapped ranges as well. > Right -- in case of exact range match the client will now get Status::Alrea Ok, sounds good to me as long as we document it in the release notes. -- To view, visit http://gerrit.cloudera.org:8080/16544 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489 Gerrit-Change-Number: 16544 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 06 Oct 2020 18:17:37 + Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16544 ) Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates .. [catalog_manager] Status::AlreadyPresent for range duplicates With this patch, CatalogManager discriminates between overlapped and exact duplicate key ranges when adding new partitions, returning Status::AlreadyPresent() for exact range duplicates and Status::InvalidArgument for otherwise overlapped ones. The number of hash buckets is not taken into account when searching/reporting about the exact duplicates in key range for partitions. Before this patch, CatalogManager returned Status::InvalidArgument() for exact duplicates and otherwise overlapped ranges as well. This patch also updates the relevant tests, so the new behavior has enough test coverage. A follow-up patch will use the newly introduced finer status reporting. To be more precise, TxnManager could send multiple requests to add the same new tablet for the transaction status table while processing concurrent BeginTransaction() requests. With that, it should be able to tell between an error when the corresponding tablet is already present and other types of errors. Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489 Reviewed-on: http://gerrit.cloudera.org:8080/16544 Tested-by: Kudu Jenkins Reviewed-by: Andrew Wong --- M java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java M src/kudu/integration-tests/alter_table-test.cc M src/kudu/integration-tests/txn_status_table-itest.cc M src/kudu/master/catalog_manager.cc 4 files changed, 148 insertions(+), 102 deletions(-) Approvals: Kudu Jenkins: Verified Andrew Wong: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/16544 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489 Gerrit-Change-Number: 16544 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 ) Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates .. Patch Set 3: Thank you for the review! -- To view, visit http://gerrit.cloudera.org:8080/16544 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489 Gerrit-Change-Number: 16544 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 06 Oct 2020 17:59:50 + Gerrit-HasComments: No
[kudu-CR] KUDU-2612 p11: persist txn metadata in superblock
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16492 to look at the new patch set (#10). Change subject: KUDU-2612 p11: persist txn metadata in superblock .. KUDU-2612 p11: persist txn metadata in superblock Currently, transaction metadata is entirely stored in the WALs; once a transaction is begun, it anchors WALs indefinitely. This is untenable, as any tablet that participates in a transaction would never be able to GC its WALs. This patch addresses this by storing transaction metadata in the tablet superblock for the following participant ops: - BEGIN_TXN: an empty record is added to the superblock for the given transaction ID. In the future, this can be used to store the owner of the transaction. - FINALIZE_COMMIT: the commit timestamp is added to the superblock for the given transaction ID. The commit timestamp of a transaction will be used when scanning over mutations applied to a tablet instead of the apply timestamp. - ABORT_TXN: an abort bit is added to the superblock for the given transaction ID. Upon applying each of these ops, the op's OpId is anchored in the WALs until the tablet metadata is successfully flushed, ensuring that if we don't flush the tablet metadata before restarting, we can rebuild any ops that whose mutations had not been persisted by replaying the WALs. What about BEGIN_COMMIT ops? While these ops will still need to be anchored, BEGIN_COMMIT ops don't need to persist any long-term information. As such, their anchoring is slightly different -- rather than mutating the tablet metadata and unanchoring on a metadata flush, BEGIN_COMMIT ops will update the in-memory state for the in-flight Txn, and unanchor once the corresponding FINALIZE_COMMIT or ABORT_TXN begins applying. On bootstrap, the following rules are applied: - If we've persisted terminal information (i.e. abort or commit) about a transaction ID, we can skip replaying any participant op for that transaction. - If we otherwise have a persisted record for the transaction ID, we should start an open transaction and replay all participant ops for that transaction. - If we have no record of a transaction persisted, we must replay all participant ops for the given transaction. While storing things in the superblock is not ideal, there are further improvements that can be made to reduce its size, e.g. after finalizing a transaction, the transactions mutations may be merged in with the rest of the tablet; once all mutations of a given transaction have been merged, the transaction metadata may be removed from the metadata. Change-Id: I2f32808aef10484f4e0ad3942bb005f61fbdb34a --- M src/kudu/integration-tests/txn_participant-itest.cc M src/kudu/tablet/metadata.proto M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/ops/participant_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_metadata-test.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tablet/tablet_metadata.h M src/kudu/tablet/txn_participant-test.cc M src/kudu/tablet/txn_participant.cc M src/kudu/tablet/txn_participant.h 13 files changed, 728 insertions(+), 131 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/16492/10 -- To view, visit http://gerrit.cloudera.org:8080/16492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f32808aef10484f4e0ad3942bb005f61fbdb34a Gerrit-Change-Number: 16492 Gerrit-PatchSet: 10 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [catalog manager] Status::AlreadyPresent for range duplicates
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16544 ) Change subject: [catalog_manager] Status::AlreadyPresent for range duplicates .. Patch Set 3: Code-Review+2 (3 comments) http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16544/2//COMMIT_MSG@25 PS2, Line 25: concurrent BeginTransaction() requests. With that, it should be able > Right -- the idea is to make sure that's TxnManager who adds the partitions Ack http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/integration-tests/alter_table-test.cc File src/kudu/integration-tests/alter_table-test.cc: http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/integration-tests/alter_table-test.cc@1420 PS2, Line 1420: // ADD [0, 100) <- already present (duplicate) > See lines 2009 and 2041 of PS2 for the coverage of such cases. Ah, thanks for the pointer! http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/16544/2/src/kudu/master/catalog_manager.cc@2467 PS2, Line 2467: elements of 'existing_ta > Done Thanks! -- To view, visit http://gerrit.cloudera.org:8080/16544 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I42c1b821f3bd6854c06682ae3c85a058665f1489 Gerrit-Change-Number: 16544 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy Gerrit-Comment-Date: Tue, 06 Oct 2020 17:51:00 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p11: persist txn metadata in superblock
Hello Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16492 to look at the new patch set (#9). Change subject: KUDU-2612 p11: persist txn metadata in superblock .. KUDU-2612 p11: persist txn metadata in superblock Currently, transaction metadata is entirely stored in the WALs; once a transaction is begun, it anchors WALs indefinitely. This is untenable, as any tablet that participates in a transaction would never be able to GC its WALs. This patch addresses this by storing transaction metadata in the tablet superblock for the following participant ops: - BEGIN_TXN: an empty record is added to the superblock for the given transaction ID. In the future, this can be used to store the owner of the transaction. - FINALIZE_COMMIT: the commit timestamp is added to the superblock for the given transaction ID. The commit timestamp of a transaction will be used when scanning over mutations applied to a tablet instead of the apply timestamp. - ABORT_TXN: an abort bit is added to the superblock for the given transaction ID. Upon applying each of these ops, the op's OpId is anchored in the WALs until the tablet metadata is successfully flushed, ensuring that if we don't flush the tablet metadata before restarting, we can rebuild any ops that whose mutations had not been persisted by replaying the WALs. What about BEGIN_COMMIT ops? While these ops will still need to be anchored, BEGIN_COMMIT ops don't need to persist any long-term information. As such, their anchoring is slightly different -- rather than mutating the tablet metadata and unanchoring on a metadata flush, BEGIN_COMMIT ops will update the in-memory state for the in-flight Txn, and unanchor once the corresponding FINALIZE_COMMIT or ABORT_TXN begins applying. On bootstrap, the following rules are applied: - If we've persisted terminal information (i.e. abort or commit) about a transaction ID, we can skip replaying any participant op for that transaction. - If we otherwise have a persisted record for the transaction ID, we should start an open transaction and replay all participant ops for that transaction. - If we have no record of a transaction persisted, we must replay all participant ops for the given transaction. While storing things in the superblock is not ideal, there are further improvements that can be made to reduce its size, e.g. after finalizing a transaction, the transactions mutations may be merged in with the rest of the tablet; once all mutations of a given transaction have been merged, the transaction metadata may be removed from the metadata. Change-Id: I2f32808aef10484f4e0ad3942bb005f61fbdb34a --- M src/kudu/integration-tests/txn_participant-itest.cc M src/kudu/tablet/metadata.proto M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/ops/participant_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_metadata-test.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tablet/tablet_metadata.h M src/kudu/tablet/txn_participant-test.cc M src/kudu/tablet/txn_participant.cc M src/kudu/tablet/txn_participant.h 13 files changed, 735 insertions(+), 130 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/16492/9 -- To view, visit http://gerrit.cloudera.org:8080/16492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I2f32808aef10484f4e0ad3942bb005f61fbdb34a Gerrit-Change-Number: 16492 Gerrit-PatchSet: 9 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2612 p10: have timestamp assignment account for commit timestamps
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16470 )
Change subject: KUDU-2612 p10: have timestamp assignment account for commit
timestamps
..
Patch Set 10:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/consensus/time_manager-test.cc
File src/kudu/consensus/time_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/consensus/time_manager-test.cc@226
PS9, Line 226: {
: // Oper
> nit: I'm not sure this adds any clarity on the expected behavior. Could yo
Done
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/consensus/time_manager-test.cc@245
PS9, Line 245: // the safe time to pass.
> nit: does it make sense to check for the result of TimeManager::AssignTimes
Done
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/consensus/time_manager.h
File src/kudu/consensus/time_manager.h:
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/consensus/time_manager.h@159
PS9, Line 159: Returns a not-OK status if called while not the leader
> nit: any reason not to mention the case when updating clock fails or that's
Done
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/tablet/ops/participant_op.h
File src/kudu/tablet/ops/participant_op.h:
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/tablet/ops/participant_op.h@75
PS9, Line 75: messag
> message
Done
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/tablet/ops/participant_op.cc
File src/kudu/tablet/ops/participant_op.cc:
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/tablet/ops/participant_op.cc@114
PS9, Line 114: DCHECK(nullptr == begin_commit_mvcc_op_);
> Is this assumed to happen only once during ParticipantOpState's lifecycle?
Done
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/tablet/tablet.cc@857
PS9, Line 857: if (op_type == tserver::ParticipantOpPB::FINALIZE_COMMIT) {
: // NOTE: we may not have an MVCC op if we are bootstrapping
and did not
: // replay a BEGIN_COMMIT op.
: i
> Is it even possible to not have commit op for FINALIZE_COMMIT operation at
Yeah, when bootstrapping. I'll leave a comment.
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/tablet/txn_participant.h
File src/kudu/tablet/txn_participant.h:
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/tablet/txn_participant.h@183
PS9, Line 183:
> Why not just SetCommitOp() as for ParticipantOpState::SetMvccOp()? Why doe
Done
http://gerrit.cloudera.org:8080/#/c/16470/9/src/kudu/tablet/txn_participant.h@228
PS9, Line 228: his ensures scans on this participant are
: // repeatable.
> nit: maybe, add some DCHECK-ing into the destructor of this Txn class to en
Done
--
To view, visit http://gerrit.cloudera.org:8080/16470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0412fd0cf778d96f3fe6b14624d8d06942f40e72
Gerrit-Change-Number: 16470
Gerrit-PatchSet: 10
Gerrit-Owner: Andrew Wong
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 06 Oct 2020 07:00:22 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2612 p10: have timestamp assignment account for commit timestamps
Hello Alexey Serbin, Kudu Jenkins, Hao Hao, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16470 to look at the new patch set (#11). Change subject: KUDU-2612 p10: have timestamp assignment account for commit timestamps .. KUDU-2612 p10: have timestamp assignment account for commit timestamps One of the hurdles to performing a transaction's commit on a participant is that the commit process must ensure repeatable reads. Without multi-op transactions, this is done via a dance between the TimeManager (the entity that tracks and assigns timestamps) and the MvccManager (the entity that tracks the lifecycle of ops). This patch extends the dance between the TimeManager and MvccManager to ensure that when a participant commits a transaction, all future ops will be assigned a higher timestamp. I found it time-consuming to peruse the existing codebase for how timestamp assignment ensured repeatable reads, so I added a block comment to time_manager.h describing how it does so. I also added a section about how this patch extends it to ensure repeatable reads in the context of transactions. Change-Id: I0412fd0cf778d96f3fe6b14624d8d06942f40e72 --- M src/kudu/consensus/consensus_queue.cc M src/kudu/consensus/time_manager-test.cc M src/kudu/consensus/time_manager.cc M src/kudu/consensus/time_manager.h M src/kudu/integration-tests/txn_participant-itest.cc M src/kudu/tablet/ops/participant_op.cc M src/kudu/tablet/ops/participant_op.h M src/kudu/tablet/ops/write_op.cc M src/kudu/tablet/ops/write_op.h M src/kudu/tablet/tablet.cc M src/kudu/tablet/tablet.h M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/txn_participant.cc M src/kudu/tablet/txn_participant.h 14 files changed, 487 insertions(+), 83 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/16470/11 -- To view, visit http://gerrit.cloudera.org:8080/16470 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0412fd0cf778d96f3fe6b14624d8d06942f40e72 Gerrit-Change-Number: 16470 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Wong Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Kudu Jenkins (120)
