Todd Lipcon has posted comments on this change. Change subject: [c++-client]: cache non-covering ranges in meta cache ......................................................................
Patch Set 1: (6 comments) didn't do a detailed look at the actual algorithm, mostly some nitty type stuff. will need someone else (or me, later) to look at the implementation. A unit test would be nice http://gerrit.cloudera.org:8080/#/c/3581/1/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: Line 62: const int MAX_RETURNED_TABLE_LOCATIONS = 10; doc this also, should this be in an anonymous namespace? (it's not used outside this compilation unit, right?) PS1, Line 313: ttl: similar to my comment on the header, this is a bit confusing, because these MonoTimes are not human-readable. Either compute a delta from 'now', or try to convert to a wall-time? PS1, Line 612: else don't need 'else' here or below on 616 because the blocks above return http://gerrit.cloudera.org:8080/#/c/3581/1/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: PS1, Line 257: { } given there's a default empty constructor defined, might be worth having a DCHECK(IsInitialized()) in all the accessors, and implement that either with a boolean member or by checking ttl_.IsInitialized() Line 283: DCHECK_NOTNULL(tablet_.get()); this will give a warning for an unused result. can just use DCHECK(tablet_); Line 316: MonoTime ttl_; really this is the 'expiration_time_' rather than the 'ttl', right? ie it's a time, not a duration -- To view, visit http://gerrit.cloudera.org:8080/3581 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I05bcb3fe05d51d7c455e1d68bd2baa6f3c2b9d21 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
