Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16031 )
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens ...................................................................... Patch Set 7: (6 comments) http://gerrit.cloudera.org:8080/#/c/16031/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16031/7//COMMIT_MSG@23 PS7, Line 23: This doesn’t avoid the need for a call to the master to get the schema in the case of writing data to Kudu, that work is tracked : by KUDU-3135. I expect the TableMetadataPB message would : be used there as well. nit: spacing http://gerrit.cloudera.org:8080/#/c/16031/7/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/7/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@202 PS7, Line 202: ProtobufHelper.pbToPartitionSchema(tableMetadata.getPartitionSchema(), schema); : table = new KuduTable(client.asyncClient, tableMetadata.getTableName(), : tableMetadata.getTableI nit: spacing in this file too http://gerrit.cloudera.org:8080/#/c/16031/7/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/16031/7/src/kudu/client/scan_token-internal.cc@122 PS7, Line 122: kuduSchema nit: snake_case http://gerrit.cloudera.org:8080/#/c/16031/7/src/kudu/client/scan_token-internal.cc@278 PS7, Line 278: kudu nit: don't need this? http://gerrit.cloudera.org:8080/#/c/16031/7/src/kudu/client/scan_token-internal.cc@279 PS7, Line 279: table_pb.mutable_schema()->CopyFrom(schema_pb); nit: can use *mutable_pb() = std::move(pb) to avoid copying. Same below. http://gerrit.cloudera.org:8080/#/c/16031/7/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: PS7: Could you also add a test that hydrates a bunch of scan tokens and checks the metric that counts the GetTableSchema calls? It'd be great if we also did the same for Spark tests via the EMC, though that seems like a fair amount of work given there's no easy way to get the metrics from the Java/Scala test utils. Maybe verify by running the jar on a real cluster with Spark/Impala at least? -- 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: 7 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: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 09 Jun 2020 04:44:43 +0000 Gerrit-HasComments: Yes
