Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13516 )
Change subject: Add seek before mode for CBTree to accelerate CheckRowDeleted in dms. ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc File src/kudu/tablet/cbtree-test.cc: http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@797 PS2, Line 797: string key = "key" + std::to_string(i * 2); nit: use Substitute("key$0", i * 2 + 1) here and below in a number of places http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@802 PS2, Line 802: exact = false; you mean exact = true, right? http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@805 PS2, Line 805: gscoped_ptr<CBTreeIterator<SmallFanoutTraits> > iter( : t.NewIterator()); nit: no need to wrap here http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@817 PS2, Line 817: faild typo http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@819 PS2, Line 819: gscoped_ptr<CBTreeIterator<SmallFanoutTraits> > iter( same style nit on wrapping (also below a few places) http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/cbtree-test.cc@828 PS2, Line 828: inexistent non-existent http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/concurrent_btree.h File src/kudu/tablet/concurrent_btree.h: http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/concurrent_btree.h@1615 PS2, Line 1615: enum SeekMode { this can be in a private: block, right? http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/concurrent_btree.h@1631 PS2, Line 1631: // Find out the first idx in the very leafnode with key <= given key. agree with Adar this is a little confusing. I don't think we really need the comment since the function is pretty straightforward and follows the one above. http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/deltamemstore.cc File src/kudu/tablet/deltamemstore.cc: http://gerrit.cloudera.org:8080/#/c/13516/2/src/kudu/tablet/deltamemstore.cc@171 PS2, Line 171: } can we add DCHECK(!exact) here? we never expect to hit an entry with Timestamp::kMax -- To view, visit http://gerrit.cloudera.org:8080/13516 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icda5585c7226a075ffebdb22c7fc7728edf85feb Gerrit-Change-Number: 13516 Gerrit-PatchSet: 2 Gerrit-Owner: ZhangYao <triplesheep0...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Yao Xu <oclarms....@gmail.com> Gerrit-Reviewer: ZhangYao <triplesheep0...@gmail.com> Gerrit-Comment-Date: Fri, 07 Jun 2019 05:01:17 +0000 Gerrit-HasComments: Yes
