Dan Burkert has posted comments on this change. Change subject: [c++-client]: cache non-covering ranges in meta cache ......................................................................
Patch Set 7: (13 comments) http://gerrit.cloudera.org:8080/#/c/3581/5/src/kudu/client/client-test.cc 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(), "", &entry)); : > You can instantiate a FlagSaver instead. Done http://gerrit.cloudera.org:8080/#/c/3581/5/src/kudu/client/client.h File src/kudu/client/client.h: Line 300: FRIEND_TEST(ClientTest, TestScanTimeout); > Nit: sort alphabetically. Done http://gerrit.cloudera.org:8080/#/c/3581/5/src/kudu/client/meta_cache.cc 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 Done PS5, Line 817: lthough the existence of A as an initial > Can you pull this (and end) into local variables? Done 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, remote->partition().partition_key_start()); : DCHECK_EQ(tablet_upper_bound, remote->partition().partition_key_end()); > 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: tablets_by_key.lower_bound(tablet_upper_bound)); > 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. http://gerrit.cloudera.org:8080/#/c/3581/5/src/kudu/client/table-internal.cc 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. http://gerrit.cloudera.org:8080/#/c/3581/5/src/kudu/master/catalog_manager.cc 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: Done http://gerrit.cloudera.org:8080/#/c/3581/5/src/kudu/tools/ksck_remote.cc 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 Done Line 320: new KsckTabletReplica(replica.ts_info().permanent_uuid(), is_leader, is_follower))); > Nit: wasn't the old indentation correct? Done -- 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
