Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17547 )
Change subject: KUDU-3291: properly disambiguate between deltas of a row with the same timestamp ...................................................................... Patch Set 3: Code-Review+1 (8 comments) LGTM overall, just a couple of questions (just to make sure my understanding is correct) and nits. http://gerrit.cloudera.org:8080/#/c/17547/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17547/3//COMMIT_MSG@36 PS3, Line 36: this counter is : propagated to the sub-iterators Just to make sure I understand this correctly: is there any guarantee that those counters would be different for sub-iterators? Is that because every selected delta increases the count? http://gerrit.cloudera.org:8080/#/c/17547/3/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/17547/3/src/kudu/tablet/delta_store.h@144 PS3, Line 144: a.disambiguator != b.disambiguator nit: wrap into PREDICT_TRUE()? http://gerrit.cloudera.org:8080/#/c/17547/3/src/kudu/tablet/delta_store.h@147 PS3, Line 147: "Could not differentiate between two deltas"; Not a part of this changelist, but does it make sense to drump information on deltas into the fatal error message? http://gerrit.cloudera.org:8080/#/c/17547/3/src/kudu/tablet/delta_store.h@473 PS3, Line 473: DCHECK_GE Can we put DCHECK_GT() here? Or it can be that no deltas are selected in a sub-iterator? http://gerrit.cloudera.org:8080/#/c/17547/3/src/kudu/tablet/diff_scan-test.cc File src/kudu/tablet/diff_scan-test.cc: http://gerrit.cloudera.org:8080/#/c/17547/3/src/kudu/tablet/diff_scan-test.cc@228 PS3, Line 228: & nit: maybe, store it by value? http://gerrit.cloudera.org:8080/#/c/17547/3/src/kudu/tablet/diff_scan-test.cc@233 PS3, Line 233: ASSERT_OK nit: could it ever fail? If yes, then it'd necessary to join the thread since line 234 wouldn't be reachable http://gerrit.cloudera.org:8080/#/c/17547/3/src/kudu/tablet/diff_scan-test.cc@243 PS3, Line 243: opts.order = ORDERED;; : opts.include_deleted_rows = true; How crucial are these custom settings for the reproduction of the issue? It would be nice to add a comment about that. http://gerrit.cloudera.org:8080/#/c/17547/3/src/kudu/tablet/diff_scan-test.cc@246 PS3, Line 246: static const bool nit: as for 'kRowKey', could use constexpr here as well (just for uniformity) -- To view, visit http://gerrit.cloudera.org:8080/17547 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iccfc518999d36679f85ed901ba65cf7b4894cd55 Gerrit-Change-Number: 17547 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 07 Jun 2021 20:17:42 +0000 Gerrit-HasComments: Yes
