Dan Burkert has posted comments on this change.

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

Patch Set 7:


File src/kudu/client/client-test.cc:

PS5, Line 1223:   // Clear the cache.
              :   meta_cache->ClearCacheForTesting();
              :   internal::MetaCacheEntry entry;
ASSERT_FALSE(meta_cache->LookupTabletByKeyFastPath(client_table_.get(), "", 
> You can instantiate a FlagSaver instead.

File src/kudu/client/client.h:

Line 300:   FRIEND_TEST(ClientTest, TestScanTimeout);
> Nit: sort alphabetically.

File src/kudu/client/meta_cache.cc:

PS5, Line 753:   if (new_status.IsServiceUnavailable()) {
             :     // One or more of the tablets is not running; retry after a 
backoff period.
             :     mutable_retrier()->DelayedRetry(this, new_status);
             :     ignore_result(delete_me.release());
             :     return;
             :   }
> To be clear, this has nothing to do with non-covering range partition suppo
Yes, that's correct.  This manifested in some of the more aggressive add/drop 
partitions tests that come in the next patch, but I thought it was more 
appropriate to fix in this patch.

Line 790:   const auto& tablet_locations = rpc.resp().tablet_locations();
> rpc.resp().tablet_locations() is called four times, maybe pull it out into 

PS5, Line 817: lthough the existence of A as an initial
> Can you pull this (and end) into local variables?

PS5, Line 848:  entry(expiration_time, last_upper_bound, tablet_lower_bound);
> This assumes that a tablet can't change in partition size (i.e. that should
We discussed this off-line and decided that the CHECKs are fine for now.  A 
malformed master response shouldn't result in memory corruption, and having a 
CHECK is good from a debugging standpoint.

PS5, Line 865:       scoped_refptr<RemoteTablet> remote = 
FindPtrOrNull(tablets_by_id_, tablet_id);
             :       if (remote.get() != nullptr) {
             :         // Partition should not have changed.
             :         DCHECK_EQ(tablet_lower_bound, 
             :         DCHECK_EQ(tablet_upper_bound, 
> Maybe encapsulate this (and the non-covered range variant) in a helper meth
I'm a little hesitant to do that because the cases are so short (just a few 
lines), every one is subtly different, and there is locking going on.

PS5, Line 874:         // Update the entry TTL.
             :         auto& entry = FindOrDie(tablets_by_k
> What if the total number of tablets is a multiple of MAX_RETURNED_TABLE_LOC
correct, that's exactly how it should work (we can't infer the presence of a 
non-covered range in that case).  It will be discovered if a lookup is 
requested for that range.

Line 885:                              
> Why FindFloorOrNull if you DCHECK_NOTNULL() right after? Seems unsafe.
There is no FindFloorOrDie.  I've updated to be a normal CHECK.  I originally 
had it as a DCHECK since it should only fire if there are bugs in the code, but 
I doubt in makes much of a performance difference.

File src/kudu/client/table-internal.cc:

PS5, Line 133:     if (s.ok()) {
             :       break;
             :     } else {
             :       LOG(WARNING) << "Error getting table locations: " << 
s.ToString() << ", retrying.";
             :     }
> Likewise, this isn't logically part of this change, right?
Right, same scenario.

File src/kudu/master/catalog_manager.cc:

Line 150: DEFINE_int32(table_locations_ttl_ms, 60 * 60 * 1000, // 1 hour
> How about expressing this as:

File src/kudu/tools/ksck_remote.cc:

Line 283:       SleepFor(MonoDelta::FromMilliseconds(100 * retries));
> That seems like a rather high sleep value. Does it cause the ksck tests to 

Line 320:           new KsckTabletReplica(replica.ts_info().permanent_uuid(), 
is_leader, is_follower)));
> Nit: wasn't the old indentation correct?

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: 7
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: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to