Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16527 )
Change subject: KUDU-2612: initial implementation of TxnManager ...................................................................... Patch Set 4: (11 comments) http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc File src/kudu/master/master.cc: http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc@97 PS3, Line 97: true > nit: perhaps keep this off until the rest of the feature lands? Though I su Right -- with lazily initialized TxnManager, this isn't an issue. We could disable this by default, but then it would be extra code to enable it in the related tests. Also, I'm not quite sure this flag is here to stay anyways -- I left it just because there was mentioned that it might be a good a idea to keep it here for a while. http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc@318 PS3, Line 318: retry_interval > nit: kConstStaticCase? Done http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc@328 PS3, Line 328: if (!txn_manager_) { : return Status::IllegalState("TxnManager is not enabled"); : } > Should we DCHECK this instead? Can we ever get here, given the other DCHECK Right, it's not possible to get here in case of a DEBUG build. I wasn't sure about how it might be called at some point, but now it's only called via InitTxnManagerTask(). Also, I think all those checks around txn_manager_ != nullptr might be gone if we remove the --txn_manager_enabled flag. http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/master.cc@336 PS3, Line 336: CHECK_EQ(state_, kRunning); > nit: reverse the order for expected and actual values Done http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/txn_manager-test.cc File src/kudu/master/txn_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/txn_manager-test.cc@71 PS3, Line 71: void SetUp() override { > nit: should we also explicitly set FLAGS_txn_manager_enabled to true? Done. I guess that would be an extra line to remove if we decide to remove the --txn_manager_enabled flag :) http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/txn_manager-test.cc@151 PS3, Line 151: > nit: exist Done http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/txn_manager.h File src/kudu/master/txn_manager.h: http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/txn_manager.h@109 PS3, Line 109: s the c > Ah, seems like it is next_txn_id_ :) Yup, after going back and forth I decided that the renaming that you suggested _and_ a comment should be good enough to make it at least a bit clearer. http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/txn_manager.proto File src/kudu/master/txn_manager.proto: http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/txn_manager.proto@43 PS3, Line 43: log messages, : // though its error code is less specific. > nit: combine lines? Done http://gerrit.cloudera.org:8080/#/c/16527/3/src/kudu/master/txn_manager.proto@93 PS3, Line 93: : > nit: spacing Done http://gerrit.cloudera.org:8080/#/c/16527/1/src/kudu/transactions/txn_system_client.cc File src/kudu/transactions/txn_system_client.cc: http://gerrit.cloudera.org:8080/#/c/16527/1/src/kudu/transactions/txn_system_client.cc@62 PS1, Line 62: if (master_addrs.empty()) { : return Status::InvalidArgument("empty list of master addresses"); : } > My real question is, should this be a CHECK or DCHECK failure instead of re Ah, I see. Right -- DCHECK/CHECK might be a better option since 'master_addrs' are expected to be empty only in case of programming error, I guess. http://gerrit.cloudera.org:8080/#/c/16527/4/src/kudu/transactions/txn_system_client.cc File src/kudu/transactions/txn_system_client.cc: http://gerrit.cloudera.org:8080/#/c/16527/4/src/kudu/transactions/txn_system_client.cc@142 PS4, Line 142: const auto ret = s.Wait(); : if (result.has_highest_seen_txn_id() && highest_seen_txn_id) { : *highest_seen_txn_id = result.highest_seen_txn_id(); : } > Should this be a part of the next patch? Or is it required in this patch? Done -- To view, visit http://gerrit.cloudera.org:8080/16527 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie952977a3ae5f625d1283389f0be8afb79df7d8c Gerrit-Change-Number: 16527 Gerrit-PatchSet: 4 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 13 Oct 2020 01:21:59 +0000 Gerrit-HasComments: Yes
