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 3: (15 comments) I did a first quick pass. So far looks reasonable, but I'm not sure I have a clear big picture here, so my comments are just some nits. 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 Not related to this changelist directly, but I'm curios: what's the benefit of storing the owner of a transaction in the superblock? As of now, we have that information stored in the transaction status table and we use it to restrict access to transaction management operations. Where we would use the information on the owner stored in the superblock? http://gerrit.cloudera.org:8080/#/c/16492/3//COMMIT_MSG@19 PS3, Line 19: commit timestamp It would be nice if you could added more colors on the intended use of the txn commit timestamp stored in the superblock. http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/metadata.proto File src/kudu/tablet/metadata.proto: http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/metadata.proto@167 PS3, Line 167: Metadata associated with each transaction that this tablet is a : // participant in It would be nice to add more colors: what they key of the map is (transaction id?) and how often this field is updated to reflect the changes in runtime? http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/ops/participant_op.h File src/kudu/tablet/ops/participant_op.h: http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/ops/participant_op.h@76 PS3, Line 76: tablet Is it worth documenting the role of the 'tablet' parameter here? http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/ops/participant_op.cc File src/kudu/tablet/ops/participant_op.cc: http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/ops/participant_op.cc@232 PS3, Line 232: txn_participant->ClearIfInitFailed(txn_id); nit: any reason not to move this after the 'if (PREDICT_FALSE(result == Op::ABORTED))' clause? 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 nit here and below: it seems the majority of methods here are documented using a different tense, e.g. here it would be 'Begin the transactions, ...'. Maybe, it's worth keeping it consistent across this file? 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: RefCounted nit: why not to use std::shared_ptr instead of scoped_refptr and use make_shared to create related instances? Why scoped_refptr is better? http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@70 PS3, Line 70: bool aborted, : boost::optional<int64_t> commit_ts Maybe, make these parameters to be 'false' and 'boost::none' by default? http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@73 PS3, Line 73: commit_ts nit: add std::move() ? http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@95 PS3, Line 95: ~TxnMetadata() {} nit: would the following be more idiomatic? ~TxnMetadata() = default; http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@446 PS3, Line 446: std::vector<std::unique_ptr<log::MinLogIndexAnchorer>> Is it possible to switch to std::vector<log::MinLogIndexAnchorer> and use move semantics for passing parameters instead using std::unique_ptr? http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@511 PS3, Line 511: txn_metadata_by_txn_id_.swap(txn_metas) nit: why 'swap' is preferred instead of 'move' here? http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@624 PS3, Line 624: anchors_needing_flush.clear(); Do you mind adding a comment to explain why it's necessary to explicitly empty the container? http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@839 PS3, Line 839: in_flight_txn_ids->swap(in_flights); nit: IIRC, we tend to uniformly use std::move() in cases like this. Why 'swap' is better for here? http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@841 PS3, Line 841: swap ditto -- 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: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Thu, 24 Sep 2020 22:14:52 +0000 Gerrit-HasComments: Yes
