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 8: (9 comments) 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: vector<Status> statuses = RunOnReplicas(followers, txn_id, op); : for (int i = 0; i < statuses.size(); i++) { : const auto& s = statuses[i > nit: it would be nice to document the reasoning behind the customization of Done http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/integration-tests/txn_participant-itest.cc@234 PS7, Line 234: ASSERT_TRUE(s.IsIllegalState()) << s.ToString(); > nit: should we also set FLAGS_maintenance_manager_polling_interval_ms to so Done 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: commit_timestamp()); > nit: does it make sense to add Done 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 Unfortunately not, it seems. I tried that in an earlier revision and the ASAN build failed to build because of it: /home/jenkins-slave/workspace/kudu-master/2/src/kudu/tablet/tablet.h:122:50: error: chosen constructor is explicit in copy-initialization 22:25:20 Status Open(const std::unordered_set<int64_t>& in_flight_txn_ids = {}); 22:25:20 ^ ~~ 22:25:20 /usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/bits/unordered_set.h:132:7: note: explicit constructor declared here 22:25:20 unordered_set(size_type __n = 10, 22:25:20 ^ 22:25:20 /home/jenkins-slave/workspace/kudu-master/2/src/kudu/tablet/tablet.h:122:50: note: passing argument to parameter 'in_flight_txn_ids' here 22:25:20 Status Open(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 > Yep, my point was to keep the tense as with the majority of comments in the Yeah, I do try to be consistent when writing new code, and if there were only a fewer updates I might've gone through with it. Perhaps in a separate patch. 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: int current_key = 0; > nit: do you mind adding an assert just before performing any txn-related pa Done http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/txn_participant-test.cc@462 PS7, Line 462: // op, and WALs should be > Not sure whether whether I'm missing it, but I cannot see the call to table Done http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/txn_participant-test.cc@468 PS7, Line 468: ASSERT_OK(tablet_replica_- > Not sure I see how 'gcable_size' could be updated before this assertion. A Nope, you didn't miss it. My mistake for not adding them. http://gerrit.cloudera.org:8080/#/c/16492/7/src/kudu/tablet/txn_participant-test.cc@506 PS7, Line 506: SSERT_OK(tablet_replica_->GetGCableDataSize(&gcable_size)); > An incomplete sentence? Done -- 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: 8 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: Thu, 01 Oct 2020 05:22:16 +0000 Gerrit-HasComments: Yes
