Adar Dembo has posted comments on this change.

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

Patch Set 1:


I just did a skim; will leave the detailed review for Todd.
File src/kudu/client/

Line 295:   return ttl_.ComesBefore(MonoTime::Now(MonoTime::COARSE)) ||
Use FINE here too.

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

Line 309:   MonoDelta ttl = ttl_.GetDeltaSince(MonoTime::Now(MonoTime::COARSE));

Line 763:   MonoTime ttl = MonoTime::Now(MonoTime::COARSE);
FINE here too.

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 
someone changes it later without realizing what they're doing.
File src/kudu/client/meta_cache.h:

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

Line 311:   std::string ToString(const KuduTable* table) const;
It's surprising for ToString() to have an argument, and for it to be a 
KuduTable at that, since the entry has a 1-1 relationship with a particular 
tablet, and a tablet is 1-1 with a table.

It's actually somewhat dangerous: what happens if I pass the wrong table in? I 
think it'll crash with a CHECK somewhere, right?

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 really 
both "cache of tablets" anymore, so the comments should be somewhat different.
File src/kudu/master/master.proto:

Line 423:   optional uint32 ttl_millis = 3 [default = 36000000];
For consistency, shouldn't this be added to GetTabletLocationsResponsePB?

Ideally the TTL and the TabletLocationsPB array would be encapsulated in one 
message, but doing that now might break backwards compatibility too much.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I05bcb3fe05d51d7c455e1d68bd2baa6f3c2b9d21
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to