Dan Burkert has posted comments on this change.

Change subject: [c++-client]: cache non-covering ranges in meta cache

Patch Set 1:


Testing is difficult since I didn't make the TTL configurable as part of this 
change (is there a way to artificially accelerate the monotime clock?).  The 
non-covering range behavior is already well tested by existing unit tests.

File src/kudu/client/meta_cache.cc:

Line 62: const int MAX_RETURNED_TABLE_LOCATIONS = 10;
> doc this

Line 295:   return ttl_.ComesBefore(MonoTime::Now(MonoTime::COARSE)) ||
> Use FINE here too.
The COARSE documentation says it's accurate to within ~1ms, which should be 
more than accurate enough for this.

Line 300:   const std::string& lower_bound = lower_bound_partition_key();
> Nit: drop std:: prefixes.

PS1, Line 313: ttl:
> similar to my comment on the header, this is a bit confusing, because these
This is a delta in ms from now.

PS1, Line 612: else
> don't need 'else' here or below on 616 because the blocks above return

Line 775:     // set an upper bound partition key on the request.
> May want to DCHECK the lack of the upper bound partition key here, in case 

File src/kudu/client/meta_cache.h:

Line 30: #include "kudu/client/client.h"
> What do you need this for?

PS1, Line 257: { }
> given there's a default empty constructor defined, might be worth having a 

Line 283:     DCHECK_NOTNULL(tablet_.get());
> this will give a warning for an unused result. can just use DCHECK(tablet_)
That doesn't compile since tablet_ is not a pointer.

Line 311:   std::string ToString(const KuduTable* table) const;
> It's surprising for ToString() to have an argument, and for it to be a Kudu
Yes, it will probably crash.  Having the table is necessary for any kind of 
smart printing, otherwise all that can be done is to print the hexified 
partition keys.  Having the proper debug printed keys makes debugging this code 
_much_ easier.

Line 316:   MonoTime ttl_;
> really this is the 'expiration_time_' rather than the 'ttl', right? ie it's

Line 412:   // Cache of tablets, keyed by table ID, then by start partition key.
> Nit: update this. Put another way, this map and tablets_by_id_ aren't reall

File src/kudu/master/master.proto:

Line 423:   optional uint32 ttl_millis = 3 [default = 36000000];
> For consistency, shouldn't this be added to GetTabletLocationsResponsePB?
I don't think there is any reason to TTL the results of GetTabletLocations 
since it will be readily apparent to the client that the locations are stale if 
RPCs are sent to the wrong location.

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: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to