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: (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 thi Done 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 to be needed or used under normal operation, but are a nice escape hatch to disabled the features. For example, we disable table metadata in the backup job due to KUDU-3146, but can still use tablet metadata. table metadata remove the need for a GetTableSchema call and tablet metadata removes the need for a GetTableLocations call. > GetTableMetadata RPC fills in tablet metadata as well. It doesn't. The source of tablet metadata is from the location cache which is populated by GetTableLocations RPC. 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? The code path is the same except that this also validates that the authz token is propagated. 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 Yes, they are independent. 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 tabl See my previous comments to Greg and my response on the includeTableMetadata comment. I could be convinced otherwise, but I think this keeps things simple for Spark. -- 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:37:49 +0000 Gerrit-HasComments: Yes
