Andrew Wong 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: (8 comments) 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 Yeah, right now, each sub-iterator only keeps track of the deltas within the delta store being iterated over. In that sense, the disambiguators assigned by each sub-iterator are correct only for that delta store. 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()? Done 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 Done 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 It is possible that no deltas are selected in the sub-iterators. 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? Done 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 si Done 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? I Done 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 uniformit This is removed. -- 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: Tue, 08 Jun 2021 00:36:20 +0000 Gerrit-HasComments: Yes
