Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14911 )
Change subject: KUDU-3021 Add metric for tablet transaction memory. ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/14911/1/src/kudu/tablet/transactions/transaction_tracker-test.cc File src/kudu/tablet/transactions/transaction_tracker-test.cc: http://gerrit.cloudera.org:8080/#/c/14911/1/src/kudu/tablet/transactions/transaction_tracker-test.cc@281 PS1, Line 281: TEST_F(TransactionTrackerTest, TestExceedAncestorsMemoryLimit) { Could you refactor this with TestTooManyTransactions to avoid duplicating all this test code? Perhaps you can make two variants of the same test, where there's just one test body whose behaviors changes slightly (or not at all) depending on which variant is running? http://gerrit.cloudera.org:8080/#/c/14911/1/src/kudu/tablet/transactions/transaction_tracker.cc File src/kudu/tablet/transactions/transaction_tracker.cc: http://gerrit.cloudera.org:8080/#/c/14911/1/src/kudu/tablet/transactions/transaction_tracker.cc@74 PS1, Line 74: "of its ancestral memory.", of an ancestral tracker. http://gerrit.cloudera.org:8080/#/c/14911/1/src/kudu/util/atomic.h File src/kudu/util/atomic.h: PS1: "Iff" isn't a typo; it means "if and only if" https://www.merriam-webster.com/dictionary/iff http://gerrit.cloudera.org:8080/#/c/14911/1/src/kudu/util/high_water_mark.h File src/kudu/util/high_water_mark.h: http://gerrit.cloudera.org:8080/#/c/14911/1/src/kudu/util/high_water_mark.h@47 PS1, Line 47: int64_t old_val = current_value(); : int64_t new_val = old_val + delta; : return new_val <= max; I appreciate the symmetry with TryIncrementBy, but I think this would be clearer as: return current_value() + delta <= max; http://gerrit.cloudera.org:8080/#/c/14911/1/src/kudu/util/mem_tracker.h File src/kudu/util/mem_tracker.h: http://gerrit.cloudera.org:8080/#/c/14911/1/src/kudu/util/mem_tracker.h@116 PS1, Line 116: // Check if this tracker itself can consume those bytes, it will not increase : // the consumption of the tracker. : // Returns true if it can. Returns true if this tracker could consume 'bytes' without exceeding its limit, false otherwise. http://gerrit.cloudera.org:8080/#/c/14911/1/src/kudu/util/mem_tracker.h@119 PS1, Line 119: bool CanConsumeByThisTracker(int64_t bytes); Maybe "CanConsumeNoAncestors" would be clearer? -- To view, visit http://gerrit.cloudera.org:8080/14911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I131b9ed27617274c1210a1678c1fdf7307b7edcc Gerrit-Change-Number: 14911 Gerrit-PatchSet: 1 Gerrit-Owner: ZhangYao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 16 Dec 2019 23:42:36 +0000 Gerrit-HasComments: Yes
