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 7: (11 comments) http://gerrit.cloudera.org:8080/#/c/16492/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16492/3//COMMIT_MSG@17 PS3, Line 17: this can be used to store the owner of : the transaction > We currently use the owner on the TxnStatusManager to ensure that only the Thank you for the clarification! http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/integration-tests/txn_participant-itest.cc File src/kudu/integration-tests/txn_participant-itest.cc: http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/integration-tests/txn_participant-itest.cc@231 PS7, Line 231: FLAGS_flush_threshold_mb = 1; : FLAGS_flush_threshold_secs = 1; : FLAGS_log_segment_size_mb = 1; nit: it would be nice to document the reasoning behind the customization of these flags http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/integration-tests/txn_participant-itest.cc@234 PS7, Line 234: NO_FATALS(TxnParticipantITest::SetUp()); nit: should we also set FLAGS_maintenance_manager_polling_interval_ms to some lower value (default is 250) ? http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/ops/participant_op.cc File src/kudu/tablet/ops/participant_op.cc: http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/ops/participant_op.cc@181 PS7, Line 181: op.finalized_commit_timestamp(); nit: does it make sense to add DCHECK(op.has_finalized_commit_timestamp()) ? http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/tablet.h@122 PS7, Line 122: std::unordered_set<int64_t> nit: could we get away with just const std::unordered_set<int64_t>& in_flight_txn_ids = {} ? http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet.h@181 PS3, Line 181: Begins > I don't think it's worth updating all such instances in such a large file, Yep, my point was to keep the tense as with the majority of comments in the file. OK, I guess it's not a big deal anyways :) http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@68 PS3, Line 68: ent state > See https://kudu.apache.org/docs/contributing.html#_pointers SGTM: it seems memory usage benefits (i.e. using 8 instead of 16 bytes) is the crux here. http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/txn_participant-test.cc File src/kudu/tablet/txn_participant-test.cc: http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/txn_participant-test.cc@448 PS7, Line 448: ASSERT_OK(tablet_replica_->GetGCableDataSize(&gcable_size)); nit: do you mind adding an assert just before performing any txn-related participant ops to make sure GC-able size was 0 at that point? http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/txn_participant-test.cc@462 PS7, Line 462: ASSERT_EQ(0, gcable_size); Not sure whether whether I'm missing it, but I cannot see the call to tablet_replica_->GetGCableDataSize() before this assertion to update 'gcable_size'. Also, if performing such a verification, should we set --log_min_segments_to_retain to unnatural low value of 0 to make sure we don't hit the case of non-finished segment in Log::AllocateSegmentAndRollOverForTests()? http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/txn_participant-test.cc@468 PS7, Line 468: ASSERT_EQ(0, gcable_size); Not sure I see how 'gcable_size' could be updated before this assertion. Am I missing something? http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/txn_participant-test.cc@506 PS7, Line 506: Test that rebuilding transaction state from the WALs and metadata An incomplete sentence? -- 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: 7 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 30 Sep 2020 04:35:43 +0000 Gerrit-HasComments: Yes
