Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16794 )
Change subject: client: allow tablet ID lookups from the MetaCache ...................................................................... Patch Set 4: (14 comments) http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/client-test.cc@2270 PS3, Line 2270: FLAGS_client_tablet_locations_by_id_ttl_ms = 25; > Will it be enough to avoid flakiness in TSAN and ASAN builds? I assume so because this is the value used in TestMetaCacheExpiry. http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/client-test.cc@2287 PS3, Line 2287: ASSERT_TRUE(meta_cache->LookupEntryByIdFastPath(tablet_id, &entry)) > nit: add an assertion on the emptiness of 'entry' after this call returns ' Done http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/client-test.cc@2349 PS3, Line 2349: // still be available. : { : internal::MetaCacheEntry entry; : ASSERT_TRUE(meta_cache->Lo > nit: here and in other places with two consecutive calls I guess it might b Done http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/client-test.cc@2362 PS3, Line 2362: ASSERT_FALSE(entry.Initialized()); > nit: set FLAGS_table_locations_ttl_ms explicitly here as well? Done http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.h@427 PS3, Line 427: ok up the locati > nit: add a comment? Done http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.h@506 PS3, Line 506: std::string* > nit: add a note on the 'remote_tablet' output parameter and its optionality Done http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@92 PS3, Line 92: client_tablet_locations_by_id_ttl_ms > Is this targeted to be a test-only flag? If so, maybe mark it as hidden? The key-based TTL is defined on the master, and sent over in the response to GetTableLocations. No such provision exists for tablet locations. I considered adding it to the protobuf, but figured that a gflag is sufficient since the TxnSystemClient will live in the tservers. It doesn't need to be test-only. I can make it so if you think it'd be wise to do so though. I used these tags since they're used for --table_locations_ttl_ms in the master. http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@608 PS3, Line 608: StatusCallbac > nit: Done http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@662 PS3, Line 662: mespace { : // Handles master-related errors and transient lookup-related errors, : // scheduling retries and returning 'true' if rescheduled, in which case, : // callers should ensure this object remains alive. Updates 'status' to include : // more information based on the response. : template <class LookupRpcClass, class RespClass> : bool RetryLookupIfNecessary(Status* status, : const RespClass& resp, : rpc::RpcRetrier* retrier, : LookupRpcClass* rpc) { : if (rpc->RetryOrReconnectIfNecessary(status)) { : return true; : } : // Handle ServiceUnavailable codes from BuildLocationsForTablet(). : if (status->ok() && resp.has_error()) { : *status = StatusFromPB(resp.error().status()); : > This seems dup with LookupRpc::SendRpcCb, do you think it is worth to gener Done, though it's not the prettiest solution ever since I had to make RetryOrReconnectIfNecessary public. http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@946 PS3, Line 946: Status MetaCache::ProcessGetTabletLocation > Is it possible to move this further, say, to line 960? Not quite that far down, but done. Also renamed UpdateTabletServer so it's clearer. http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@970 PS3, Line 970: DCHECK_EQ(tablet_upper_bound, remote->partition().partition_key_end()); : : VLOG(3) << "Refreshing tablet " << tablet_id << ": " << SecureShortDebugString(tablet); : RETURN_NOT_OK_PREPEND(remote->Refresh(ts_cache_, tablet, ts_infos), : Substitute("failed to refresh locations for tablet $0", : tablet_id)); : MetaCacheEntry entry(expiration_time, remote); : a > Could LookupOrEmplace() be a better choice here to avoid double lookup (eff Done http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@981 PS3, Line 981: else { > Is it going to be a memory leak if Refresh at the next line returns non-OK 'remote' is a scoped_ptr. I can make that explicit above instead of using auto. http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@1089 PS3, Line 1089: ablet_lower_bound, remote->partition().partition_key_start()); > nit: maybe, update this using Substitute() if touching this line? Done http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@1103 PS3, Line 1103: > nit: use Substitute() here for better readability? Done -- To view, visit http://gerrit.cloudera.org:8080/16794 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib2333add5c3ab8403c48e69d29c90f3aec0914b6 Gerrit-Change-Number: 16794 Gerrit-PatchSet: 4 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 04 Dec 2020 00:46:58 +0000 Gerrit-HasComments: Yes
