Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16043 )
Change subject: KUDU-2612 p1: add initial transaction status storage ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet-test.cc File src/kudu/transactions/status_tablet-test.cc: http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet-test.cc@80 PS2, Line 80: return { txn_id, std::move(user), std::move(txn_pb), std::move(prt_pbs) }; > warning: parameter 'user' is passed by value and only copied once; consider Done http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h File src/kudu/transactions/status_tablet.h: http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@45 PS2, Line 45: // Signal to the visitor that a transaction exists with the given transaction > Could you add a comment about this function? Done http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@63 PS2, Line 63: a : // transaction entry, participant entry, etc. > I don't understand why getting lowest value is easier than getting highest I'll remove the negative for now since I don't think it's useful to mention it yet, but the gist is that Kudu has the ability to perform an ordered scan, returning results lowest to highest. If this field is negative, finding the lowest value is as simple as performing an ordered scan with a limit on results size of 1. http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@93 PS2, Line 93: // contents of the tablet into a more usable memory representation. > warning: single-argument constructors must be marked explicit to avoid unin Done http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.h@96 PS2, Line 96: // Writes to the underlying storage. Returns an error if there was an error : // writing the new entry. > I'll take a look below but I don't understand the purpose of this function. Updated the comment; I suppose it's clearer in the context of p2 of the patch series. http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc File src/kudu/transactions/status_tablet.cc: http://gerrit.cloudera.org:8080/#/c/16043/2/src/kudu/transactions/status_tablet.cc@78 PS2, Line 78: oid ExtractKeys > Any reason to keep these as standalone functions v/s making them private st I don't expect them to be used outside this compilation unit, so I don't see a reason to tie them to the TxnStatusTablet class. -- To view, visit http://gerrit.cloudera.org:8080/16043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I94ddbd37c65932120835d6e138307f819935173c Gerrit-Change-Number: 16043 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 09 Jun 2020 23:19:21 +0000 Gerrit-HasComments: Yes
