Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16031 )
Change subject: KUDU-1802: Avoid calls to master when using scan tokens ...................................................................... Patch Set 15: (10 comments) http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.h@2661 PS14, Line 2661: include_metadata > include_tablet_metadata ? Done http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto File src/kudu/client/client.proto: http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto@44 PS14, Line 44: // Metdata about a single server. : // This can be used on the client to update the local cache of where : // each server is located. : message ServerMetadataPB { : optional bytes uuid = 1; : : > nit: add a doc for this message? Done http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto@103 PS14, Line 103: > add [(kudu.REDACT) = true]? I don't think we need to. It's schema not data. We don't redact projected_columns, which is very similar. http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto@107 PS14, Line 107: optional TableMetadataPB table_metadata = 21; > ditto Same as above. http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/meta_cache.h@341 PS14, Line 341: const MonoTime& expiration_time() const { > add 'const' specifier? Done http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@158 PS14, Line 158: entry > Does it make sense to check for the staleness of an entry and populate the LookupEntryByKeyFastPath will return false if the entry is stale. Good catch. I do put the ttl on the metadata and propagate it in the java client. I meant to do it here too. http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@206 PS14, Line 206: e > no need to have reference for int, I think. Done http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@210 PS14, Line 210: > style int: stick '&' to the type Done http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@210 PS14, Line 210: { > nit: replace with I like to see the type coming from the protobuf messages. Given it's generated code, I find it can make finding usages via grep easier too. http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-test.cc@785 PS14, Line 785: aut > nit here and below: auto Done -- To view, visit http://gerrit.cloudera.org:8080/16031 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b Gerrit-Change-Number: 16031 Gerrit-PatchSet: 15 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Greg Solovyev <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 25 Jun 2020 01:09:46 +0000 Gerrit-HasComments: Yes
