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 14: (5 comments) 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@173 PS14, Line 173: List<Integer> columns = new ArrayList<>(message.getProjectedColumnsCount()); : for (Common.ColumnSchemaPB colSchemaFromPb : message.getProjectedColumnsList()) { : int colIdx = colSchemaFromPb.hasId() && schema.hasColumnIds() ? : schema.getColumnIndex(colSchemaFromPb.getId()) : : schema.getColumnIndex(colSchemaFromPb.getName()); : ColumnSchema colSchema = schema.getColumnByIndex(colIdx); : if (colSchemaFromPb.getType() != : colSchema.getType().getDataType(colSchema.getTypeAttributes())) { : throw new IllegalStateException(String.format( : "invalid type %s for column '%s' in scan token, expected: %s", : colSchemaFromPb.getType().name(), colSchemaFromPb.getName(), : colSchema.getType().name())); : } : if (colSchemaFromPb.getIsNullable() != colSchema.isNullable()) { : throw new IllegalStateException(String.format( : "invalid nullability for column '%s' in scan token, expected: %s", : colSchemaFromPb.getName(), colSchema.isNullable() ? "NULLABLE" : "NOT NULL")); : } : columns.add(colIdx); : } style/readbility nit: maybe, instead of putting this under 'else', move this out of the 'else' clause? 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. Could you extend on the details of table vs tablet metadata here? I.e., what table metadata includes. Does the inclusion of the table metadata means including tablet metadata as well? I think the source of my confusion comes from the fact that GetTableMetadata RPC fills in tablet metadata as well. 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"); Does this mean the non-enforced authz code path are no longer covered? 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) Is it possible to have .includeTabletMetadata(false) .includeTableMetadata(true) ? If not, maybe add an explicit case to verify the behavior of the builder? http://gerrit.cloudera.org:8080/#/c/16031/14/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala: http://gerrit.cloudera.org:8080/#/c/16031/14/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala@83 PS14, Line 83: builder.includeTableMetadata(options.useDriverMetadata) : builder.includeTabletMetadata(options.useDriverMetadata) Do we have use cases where false,true or true,false pairs are used for table,tablet metadata in a token at all? In other words, why not to provide controls for different properties for Kudu RDD? If it doesn't make much sense, maybe that means we just need one control knob for both table and tablet metadata inclusion in scan tokens? -- 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: Wed, 24 Jun 2020 22:17:06 +0000 Gerrit-HasComments: Yes
