Alexey Serbin 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: (9 comments) http://gerrit.cloudera.org:8080/#/c/16031/15//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16031/15//COMMIT_MSG@24 PS15, Line 24: resiliancy resiliency http://gerrit.cloudera.org:8080/#/c/16031/15//COMMIT_MSG@35 PS15, Line 35: functionlity functionality http://gerrit.cloudera.org:8080/#/c/16031/15//COMMIT_MSG@37 PS15, Line 37: unforseen unforeseen http://gerrit.cloudera.org:8080/#/c/16031/14/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java: http://gerrit.cloudera.org:8080/#/c/16031/14/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@441 PS14, Line 441: * If the table metadata is included on the scan token a GetTableSchema : * RPC call to the master can be avoided when deserializing each scan token : * into a scanner. > They are separate and can be toggled separately. The toggles are expected t Yes, right -- I meant GetTableLocations RPC populate tablet location information (i.e. tablet metadata in this context). You are right: there isn't GetTableMetadata RPC in Kudu. http://gerrit.cloudera.org:8080/#/c/16031/14/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java: http://gerrit.cloudera.org:8080/#/c/16031/14/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@66 PS14, Line 66: .addTabletServerFlag("--tserver_enforce_access_control=true"); > The code path is the same except that this also validates that the authz to Great. Do we have assurance that the code paths for --tserver_enforce_access_control=false are covered w.r.t. the newly introduced functionality? http://gerrit.cloudera.org:8080/#/c/16031/14/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@636 PS14, Line 636: .includeTabletMetadata(true) : .includeTableMetadata(false) > Yes, they are independent. OK, sounds good to me. Do you think it's worth adding a small test case to verify that a builder with .includeTabletMetadata(false) .includeTableMetadata(true) works as expected? 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@103 PS14, Line 103: > I don't think we need to. It's schema not data. We don't redact projected_c Indeed: SGTM. http://gerrit.cloudera.org:8080/#/c/16031/15/src/kudu/client/client.proto File src/kudu/client/client.proto: http://gerrit.cloudera.org:8080/#/c/16031/15/src/kudu/client/client.proto@76 PS15, Line 76: // Cached table locations should not live longer than this timeout. : optional uint64 ttl_millis = 5; Ideally, we should have set the expiration date in absolute terms given how the tokens are generated and used: they might spend some time 'on the shelf' by the time they are used. Using TTL means we assume that time interval is short. Do you think it's worth adding a doc blurb about this and what client does when it finds the metadata is expired? 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 > LookupEntryByKeyFastPath will return false if the entry is stale. Ah, indeed: LookupEntryByKeyFastPath() returns false for stale entries. -- 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 02:41:54 +0000 Gerrit-HasComments: Yes
