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 14: (6 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 Done http://gerrit.cloudera.org:8080/#/c/16031/15//COMMIT_MSG@35 PS15, Line 35: functionlity > functionality Done http://gerrit.cloudera.org:8080/#/c/16031/15//COMMIT_MSG@37 PS15, Line 37: unforseen > unforeseen Done 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"); > Great. Do we have assurance that the code paths for --tserver_enforce_acce I am not sure what you mean. The only difference I am aware of is the client validating the authz token. That code path is a superset of the code path that does not check for authz tokens. In this specific test, validating the authz token could result in an extra GetTableSchema call (if it's missing), but not validating it has no impact on the 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) > OK, sounds good to me. Sure I can add it to the size test. 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: // Serialization format for client scan tokens. Scan tokens are serializable : // scan descriptors that are used > Ideally, we should have set the expiration date in absolute terms given how I will add a docs blurb. The issue with using something other than the TTL is it would need to assume the clocks are in sync on the node that generates the token and the node that uses the token which can't necessarily be true. Extending the TTL in this scenario has little downside given the scan token has already been generated based on the metadata at this point in time and the client can retry if anything has changed. -- 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: 14 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 03:04:38 +0000 Gerrit-HasComments: Yes
