Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13536 )
Change subject: KUDU-2809 (4/6): skip unobservable rows when iterating a DRS ...................................................................... Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/13536/2/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/13536/2/src/kudu/tablet/delta_store.h@133 PS2, Line 133: // Comparator that establishes a total ordering amongst Deltas. > Obvious detail worth mentioning: "a total ordering amongst Deltas for the s Done http://gerrit.cloudera.org:8080/#/c/13536/2/src/kudu/tablet/delta_store.cc File src/kudu/tablet/delta_store.cc: http://gerrit.cloudera.org:8080/#/c/13536/2/src/kudu/tablet/delta_store.cc@114 PS2, Line 114: // delta type | oldest | newest > Thanks for including this helpful block comment. Ack http://gerrit.cloudera.org:8080/#/c/13536/2/src/kudu/tablet/delta_store.cc@156 PS2, Line 156: existing->newest > Why include newest? When could existing->oldest not be less than existing-> Good question. Looking at it again I agree that it can be simplified. http://gerrit.cloudera.org:8080/#/c/13536/2/src/kudu/tablet/delta_store.cc@158 PS2, Line 158: existing->oldest > same Done http://gerrit.cloudera.org:8080/#/c/13536/2/src/kudu/tablet/delta_store.cc@278 PS2, Line 278: Traits::kType, > nit: Seems worth writing a constructor to guarantee all fields get set so w Gonna pass. There's only this one construction site, and I get a compiler error if I remove any of the fields except the last one. Plus adding a constructor means I need to rename all of the fields in order to disambiguate vs. the constructor args. http://gerrit.cloudera.org:8080/#/c/13536/2/src/kudu/tablet/delta_store.cc@281 PS2, Line 281: decoder.get_type() }; > Should be: Hmm, but within Delta I do want to treat these as int64_t (i.e. a 64-bit integer that uniquely identifies a delta store). That is, I want to hide the fact that this is a pointer to something. And I don't think size_t makes sense as this isn't a byte or length quantity. -- To view, visit http://gerrit.cloudera.org:8080/13536 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2b616c4e8b99dbc7063b940bcb35352f87ab0226 Gerrit-Change-Number: 13536 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 07 Jun 2019 07:49:51 +0000 Gerrit-HasComments: Yes
