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 5: (15 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 > Not related to this changelist directly, but I'm curios: what's the benefit We currently use the owner on the TxnStatusManager to ensure that only the owner performs DDL on the transaction. I think it also may make sense to ensure only the owner performs DML on participants, in which case we either need to persist the owner on disk, or fetch it from the TxnStatusManager upon starting up. 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 Done 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: Map from txn ID to metadata associated with the transaction. This is : // updated on eac > It would be nice to add more colors: what they key of the map is (transacti Done 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: data. > Is it worth documenting the role of the 'tablet' parameter here? Done 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: return; > nit: any reason not to move this after the 'if (PREDICT_FALSE(result == Op: If you're referring to the ClearIfInitFailed(), an initialization failure can only happen if we've aborted -- the only state change that can leave us here is if we try to BEGIN_TXN and abort, we'd start with kInitialized and fail to get to kOpen. Added a comment. 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 us I don't think it's worth updating all such instances in such a large file, especially if there isn't a clear winner between tenses. I personally try to stick to describing the method when writing comments about methods, and using imperative tense when writing in-line comments about what certain code snippets are doing. 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_s See https://kudu.apache.org/docs/contributing.html#_pointers The scoped_refptr stores the counter with the object. The main benefit I see for shared_ptr is the weak_ptr use-case, which I don't expect to have here. http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@70 PS3, Line 70: bool aborted = false, : boost::optional<int64_t> commit_ts > Maybe, make these parameters to be 'false' and 'boost::none' by default? Done http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.h@73 PS3, Line 73: std::move > nit: add std::move() ? Done 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? Done 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 m Not currently; MinLogIndexAnchorer doesn't allow copies or assignment. Defining a move constructor seems non-trivial. 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_ = std::move(txn > nit: why 'swap' is preferred instead of 'move' here? Replied below. http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@624 PS3, Line 624: // Now that we've flushed, we > Do you mind adding a comment to explain why it's necessary to explicitly em Done http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@839 PS3, Line 839: } > nit: IIRC, we tend to uniformly use std::move() in cases like this. Why 's I was thinking we might not want to call any destructors under lock, as it adds more work, but it might be a needless optimization. Done http://gerrit.cloudera.org:8080/#/c/16492/3/src/kudu/tablet/tablet_metadata.cc@841 PS3, Line 841: ) { > ditto 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: 5 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 29 Sep 2020 00:12:19 +0000 Gerrit-HasComments: Yes
