Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11395 )
Change subject: KUDU-686 (part 2/2): use DeltaPreparer in DeltaFileIterator ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.h@293 PS3, Line 293: MaybeProcessRowChange > How about rename and slightly modify to be a bit more explicit about the se Done, except for the explicit description of the states that are compared; that feels like too much detail to me. http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.h@305 PS3, Line 305: rowid_t > How about boost::optional<rowid_t> to avoid having to check for the sentine Done http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc File src/kudu/tablet/delta_store.cc: http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@59 PS3, Line 59: with a higher > at the given or a higher Documenting this correctly is pretty though. I'll go the other way and elide the details, since the code is just below and it's only a few lines long. http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@60 PS3, Line 60: false; it's set to true > this looks reversed to me See above. http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@165 PS3, Line 165: MaybeProcessRowChange(key.row_idx()); > It seems like this should be done after the IsDeltaRelevant() check Huh. I explicitly put this here because I had seen test failures when it was below, and I even rationalized it. Of course, when I moved it after the check to retest, I can't repro those failures, nor can I remember my rationalization. I'll move it. http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@178 PS3, Line 178: if (!Traits::kAllowReinserts && decoder.is_reinsert()) { > nit: PREDICT_FALSE? Before he left Todd mentioned that the compiler will optimize this such that, in the specialization where it's true, it'll be kept, and in the specialization where it's false, the entire block will be removed. As for the delta file case (where reinserts are allowed), why would we PREDICT_FALSE? It's not like we know with certainty that REINSERTs are rare; that's workload dependent. http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@182 PS3, Line 182: UpdateDeletionState(decoder.get_type()); > Can we call MaybeProcessRowChange() from within UpdateDeletionState() ? I'd rather not; it'd mean passing the row_idx into UpdateDeletionState(). Plus I think calling MaybeProcessRowChange only out of the main control flow methods (i.e. AddDelta and Finish) helps clarify its usage. http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@223 PS3, Line 223: last_added_idx_ = key.row_idx(); > Can we call UpdateDeletionState() down here at the end? It would be better Not without some hackery; in the PREPARED_FOR_COLLECT case we haven't decoded the changelist, so we don't know whether it's a delete or not. http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/delta_store.cc@337 PS3, Line 337: last_added_idx_ != MathLimits<rowid_t>::kMax > I'm not a big fan of this... see my comment in the header file about using Changed to optional. http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/deltafile.cc File src/kudu/tablet/deltafile.cc: http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/deltafile.cc@623 PS3, Line 623: visitor > AddDelta() call Done http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/deltamemstore.cc File src/kudu/tablet/deltamemstore.cc: http://gerrit.cloudera.org:8080/#/c/11395/3/src/kudu/tablet/deltamemstore.cc@248 PS3, Line 248: // TODO(adar): is this correct? > I think this comment should be: Done. I left a TODO here for myself to also evaluate the optimization; I think I can prepare a test case that both proves its correctness and checks whether it's worth doing. Will report back. -- To view, visit http://gerrit.cloudera.org:8080/11395 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I87de52092262c4b42c1bd5107f9139edfc3888b5 Gerrit-Change-Number: 11395 Gerrit-PatchSet: 3 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Sat, 15 Sep 2018 01:54:35 +0000 Gerrit-HasComments: Yes
