David Ribeiro Alves 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 4: (5 comments) I think we should have at least a unit test to test the early out stuff, hopefully an microbench too. http://gerrit.cloudera.org:8080/#/c/11395/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11395/4//COMMIT_MSG@10 PS4, Line 10: service what's service? http://gerrit.cloudera.org:8080/#/c/11395/4//COMMIT_MSG@27 PS4, Line 27: I : don't have a suitable microbenchmark to prove this, always skeptical of perf improvements that are not measured (and tracked to see whether they changed). maybe theres some test on the dashboard that we could reuse/adapt? we need to have more coverage for large tables anyway. http://gerrit.cloudera.org:8080/#/c/11395/4/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/11395/4/src/kudu/tablet/delta_store.h@274 PS4, Line 274: // Call when a new delta becomes available in DeltaIterator::PrepareBatch. nit: maybe move this part before the paragraph above? http://gerrit.cloudera.org:8080/#/c/11395/4/src/kudu/tablet/delta_store.cc File src/kudu/tablet/delta_store.cc: http://gerrit.cloudera.org:8080/#/c/11395/4/src/kudu/tablet/delta_store.cc@95 PS4, Line 95: MvccSnapshot GetFullyInclusiveSnapshot(); missing docs these seem to be redundant, essentially an in class rename of the mvcc methods that requires the reader to now understand what "fullyinclusive" means http://gerrit.cloudera.org:8080/#/c/11395/4/src/kudu/tablet/delta_store.cc@233 PS4, Line 233: DCHECK_LE(cur_prepared_idx_ - prev_prepared_idx_, dst->nrows()); why again don't these match anymore? maybe doc it? -- 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: 4 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Mon, 17 Sep 2018 16:16:31 +0000 Gerrit-HasComments: Yes
