Mike Percy 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 2: (6 comments) I'm working through this patch... as it's changing some core code I've never worked with before, I am just posting some initial thoughts. http://gerrit.cloudera.org:8080/#/c/11395/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11395/2//COMMIT_MSG@7 PS2, Line 7: use DeltaPreparer Can you talk more about how this changes the logic? Or do you view this as simply a refactor? Can you also put this in context of diff scans and explain how this helps? http://gerrit.cloudera.org:8080/#/c/11395/2//COMMIT_MSG@9 PS2, Line 9: leverage DeltaPreparer why? just code reuse? get rid of Todd's TODO in the code? http://gerrit.cloudera.org:8080/#/c/11395/2//COMMIT_MSG@25 PS2, Line 25: improvements Can you be more specific about what we think these improvements are? http://gerrit.cloudera.org:8080/#/c/11395/2/src/kudu/tablet/delta_store.h File src/kudu/tablet/delta_store.h: http://gerrit.cloudera.org:8080/#/c/11395/2/src/kudu/tablet/delta_store.h@258 PS2, Line 258: finished_row nit: doc this parameter http://gerrit.cloudera.org:8080/#/c/11395/2/src/kudu/tablet/delta_store.cc File src/kudu/tablet/delta_store.cc: http://gerrit.cloudera.org:8080/#/c/11395/2/src/kudu/tablet/delta_store.cc@57 PS2, Line 57: template<DeltaType Type> Mind documenting the interface to this helper? It's not obvious what the meaning of 'finished_row' is in this context. http://gerrit.cloudera.org:8080/#/c/11395/2/src/kudu/tablet/deltamemstore.cc File src/kudu/tablet/deltamemstore.cc: http://gerrit.cloudera.org:8080/#/c/11395/2/src/kudu/tablet/deltamemstore.cc@248 PS2, Line 248: // TODO(adar): is this correct? Mind adding a comment with the thought process behind this bookkeeping / cleanup code? -- 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: 2 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: Wed, 12 Sep 2018 17:26:28 +0000 Gerrit-HasComments: Yes
