Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16358 )
Change subject: KUDU-2612 p9: anchor participant ops in WAL ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/consensus/log_anchor_registry.cc File src/kudu/consensus/log_anchor_registry.cc: http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/consensus/log_anchor_registry.cc@63 PS3, Line 63: : Status LogAnchorRegistry::Unregister(LogAnchor* anchor) { : std::lock_guard<simple_spinlock> l(lock_); : return UnregisterUnlocked(anchor); : } : : Status LogAnchorRegistry::UnregisterIfAnchored(LogAnchor* anchor) { : std::lock_guard<simple_spinlock> l(lock_); : if (!anchor->is_registered) return Status::OK(); : > Once RegisterOrUpdate() is introduced, maybe it's time to get rid of Update Done http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/gutil/map-util.h File src/kudu/gutil/map-util.h: http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/gutil/map-util.h@619 PS3, Line 619: constructor argume > I guess this needs to be updated as well? Done http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/tablet_replica-test-base.h File src/kudu/tablet/tablet_replica-test-base.h: http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/tablet_replica-test-base.h@72 PS3, Line 72: is for ca > nit: is for cases Done http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/tablet_replica-test-base.cc File src/kudu/tablet/tablet_replica-test-base.cc: http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/tablet_replica-test-base.cc@83 PS3, Line 83: > nit: these parentheses might be omitted Done http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/txn_participant-test.cc File src/kudu/tablet/txn_participant-test.cc: http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/txn_participant-test.cc@84 PS3, Line 84: > nit: need a finer-grained This reads correctly as is: we disable anything that might write to disk so that we have finer-grained control of the on-disk state. http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/txn_participant-test.cc@351 PS3, Line 351: > nit: pass this by a cont reference? Done http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/txn_participant-test.cc@439 PS3, Line 439: (tablet_replica_- > nit here and below: do you mind specifying the expected value first in ASSE Done http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/txn_participant-test.cc@458 PS3, Line 458: // NOTE: we need to reset the tablet here to reset the TxnParticipant. > It seems the meat of this test scenario was stolen by the underpants gnomes Indeed, the gnomes stole it right from out of my brain before I had a chance to write this :) http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/txn_participant.h File src/kudu/tablet/txn_participant.h: http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/txn_participant.h@84 PS3, Line 84: onstructs a transaction instance with the given transaction > Is it worth documenting these parameters? Done http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/txn_participant.cc File src/kudu/tablet/txn_participant.cc: http://gerrit.cloudera.org:8080/#/c/16358/3/src/kudu/tablet/txn_participant.cc@64 PS3, Line 64: txn > What's the meaning of not finding the required transaction? Could the perm It means that the transaction ID has already been cleared. The permissive behavior is inconsequential in current test usage, but I can make it return a value to be conservative. -- To view, visit http://gerrit.cloudera.org:8080/16358 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I936f0a345c4b6095f0d99b6dd244e3092ae3f9d7 Gerrit-Change-Number: 16358 Gerrit-PatchSet: 4 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: Mon, 31 Aug 2020 23:32:09 +0000 Gerrit-HasComments: Yes
