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 13: (12 comments) http://gerrit.cloudera.org:8080/#/c/16031/12/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/12/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala@83 PS12, Line 83: builder.includeTableMetadata(options.useDriverMetadata) > Just curious, why we have 2 separate flags in Java client, but only one fla I wanted to provide more fine-grained control when using the direct API because custom applications might want more control. With the Spark integration I couldn't see a reason to have them separate. The spark configuration is more of a toggle to turn the entire feature on or off in case there is an issue with large tokens. I could be convinced to combine them here too. http://gerrit.cloudera.org:8080/#/c/16031/12/src/kudu/client/meta_cache.h File src/kudu/client/meta_cache.h: http://gerrit.cloudera.org:8080/#/c/16031/12/src/kudu/client/meta_cache.h@422 PS12, Line 422: > warning: missing username/bug in TODO [google-readability-todo] Done http://gerrit.cloudera.org:8080/#/c/16031/10/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/16031/10/src/kudu/client/scan_token-internal.cc@282 PS10, Line 282: message.snap_timestamp())); > nit: use std::move() here as well? Done http://gerrit.cloudera.org:8080/#/c/16031/11/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/16031/11/src/kudu/client/scan_token-internal.cc@71 PS11, Line 71: > warning: using decl 'HostPortPB' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/16031/11/src/kudu/client/scan_token-internal.cc@77 PS11, Line 77: > warning: using decl 'LookupRpc' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/16031/11/src/kudu/client/scan_token-internal.cc@150 PS11, Line 150: // Prime the client tablet location cache if no entry is already present. > warning: the variable 'tablet_metadata' is copy-constructed from a const re Done http://gerrit.cloudera.org:8080/#/c/16031/11/src/kudu/client/scan_token-internal.cc@170 PS11, Line 170: replica_pb.set_ts_info_idx(replica_meta.ts_idx()); > warning: the 'empty' method should be used to check for emptiness instead o Done http://gerrit.cloudera.org:8080/#/c/16031/11/src/kudu/client/scan_token-internal.cc@171 PS11, Line 171: replica_pb.set_role(replica_meta.role()); > warning: narrowing conversion from 'unsigned int' to signed type 'int' is i Done http://gerrit.cloudera.org:8080/#/c/16031/11/src/kudu/client/scan_token-internal.cc@171 PS11, Line 171: replica_pb.set_role(replica_meta.role()); > warning: the type of the loop variable 'column_idx' is different from the o Done http://gerrit.cloudera.org:8080/#/c/16031/13/src/kudu/client/scan_token-internal.cc File src/kudu/client/scan_token-internal.cc: http://gerrit.cloudera.org:8080/#/c/16031/13/src/kudu/client/scan_token-internal.cc@184 PS13, Line 184: for (const HostPortPB host_port :server_meta.rpc_addresses()) { > warning: the loop variable's type is not a reference type; this creates a c Done http://gerrit.cloudera.org:8080/#/c/16031/10/src/kudu/client/scan_token-test.cc File src/kudu/client/scan_token-test.cc: http://gerrit.cloudera.org:8080/#/c/16031/10/src/kudu/client/scan_token-test.cc@759 PS10, Line 759: shared_ptr<KuduTable> table; > nit: add 'const' ? Done http://gerrit.cloudera.org:8080/#/c/16031/10/src/kudu/client/scan_token-test.cc@764 PS10, Line 764: .set_range_partition_columns({}) : > nit: for better readability, maybe introduce a variable for this (a const p Done -- 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: 13 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 18:02:01 +0000 Gerrit-HasComments: Yes
