Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16794 )
Change subject: client: allow tablet ID lookups from the MetaCache ...................................................................... Patch Set 3: (12 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? http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/client-test.cc@2287 PS3, Line 2287: ASSERT_FALSE(meta_cache->LookupEntryByIdFastPath(tablet_id, &entry)); nit: add an assertion on the emptiness of 'entry' after this call returns 'false' http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/client-test.cc@2349 PS3, Line 2349: ASSERT_TRUE(meta_cache->LookupEntryByKeyFastPath(client_table_.get(), "", &entry)); : ASSERT_FALSE(entry.stale()); : ASSERT_TRUE(meta_cache->LookupEntryByIdFastPath(first_tablet_id, &entry)); : ASSERT_FALSE(entry.stale()); nit: here and in other places with two consecutive calls I guess it might be safer to use separate variables for 'entry'. http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/client-test.cc@2362 PS3, Line 2362: FLAGS_client_tablet_locations_by_id_ttl_ms = 25; nit: set FLAGS_table_locations_ttl_ms explicitly here as well? 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@506 PS3, Line 506: OK if the lookup was successful nit: add a note on the 'remote_tablet' output parameter and its optionality 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 > Sorry, not directly related to your change, but do you think such flag make Is this targeted to be a test-only flag? If so, maybe mark it as hidden? http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@608 PS3, Line 608: virtual ~LookupRpcById() {} nit: virtual ~LookupRpcById() = default; ? http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@946 PS3, Line 946: std::lock_guard<percpu_rwlock> l(lock_); Is it possible to move this further, say, to line 960? http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@970 PS3, Line 970: auto* entry = FindOrNull(entry_by_tablet_id_, tablet_id); : if (entry) { : entry->refresh_expiration_time(expiration_time); : } else { : MetaCacheEntry entry(expiration_time, remote); : VLOG(3) << "Caching '" << tablet_id << "' entry"; : EmplaceOrDie(&entry_by_tablet_id_, tablet_id, std::move(entry)); : } Could LookupOrEmplace() be a better choice here to avoid double lookup (effectively) when an entry isn't present? http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@981 PS3, Line 981: remote Is it going to be a memory leak if Refresh at the next line returns non-OK status? Maybe, wrap this into unique_ptr right here? http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@1089 PS3, Line 1089: "Refreshing tablet " << tablet_id << ": " << SecureShortDebugString(tablet); nit: maybe, update this using Substitute() if touching this line? http://gerrit.cloudera.org:8080/#/c/16794/3/src/kudu/client/meta_cache.cc@1103 PS3, Line 1103: "Caching '" << table->name() << "' entry " << entry.DebugString(table); nit: use Substitute() here for better readability? -- 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: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 03 Dec 2020 21:49:33 +0000 Gerrit-HasComments: Yes
