You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2020/06/04 17:47:37 UTC
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16031
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
KUDU-1802: Avoid call to master when deserializing scan tokens
This patch adds a new table metadata field to the scan token
which contains all of the metadata required to construct a
KuduTable in the clients. This means an RPC call to the
master is no longer required when deserializing the scan token.
The old table name and table id fields are used as a fallback to
maintain backwards compatibility.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, should
allow for table metadata to be leveraged in those cases as well.
This doesn’t avoid the need for a call to the master in the case
of producing data to Kudu, that work is tracked by KUDU-3135.
I expect the TableMetadataPB message would be used there as
well.
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
10 files changed, 212 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/1
--
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: newchange
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#16).
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
KUDU-1802: Avoid calls to master when using scan tokens
This patch adds new metadata to the scan token to allow it
to contain all of the metadata required to construct a KuduTable
and open a scanner in the clients. This means the GetTableSchema
and GetTableLocations RPC calls to the master are no longer required
when using the scan token.
New TableMetadataPB, TabletMetadataPB, and authorization token
fields were added as optional fields on the token. Additionally a
`projected_column_idx` field was added that can be used in place
of the `projected_columns`. This significantly reduces the size of
the scan token by not duplicating the ColumnSchemaPB that is
already in the TableMetadataPB.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliency to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, tracked
by KUDU-3146, should allow for table metadata to be leveraged in
those cases as well.
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.
I included the ability to disable this functionality in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforeseen issues or regressions with
this feature.
I added a test to compare the serialized size of the scan token with
and without the table and tablet metadata. The size results for a
100 column table are:
no metadata: 2697 Bytes
tablet metadata: 2805
tablet, table, and authz metadata: 3258
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
19 files changed, 1,018 insertions(+), 136 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/16
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 16
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jun 2020 22:37:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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 5:
(7 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 in the case
: of writing data to Kudu, that work is tracked by KUDU-3135.
: I expect the TableMeta
> nit: spacing
Done
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
Done
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
Done
http://gerrit.cloudera.org:8080/#/c/16031/7/src/kudu/client/scan_token-internal.cc@278
PS7, Line 278: le_s
> nit: don't need this?
Done
http://gerrit.cloudera.org:8080/#/c/16031/7/src/kudu/client/scan_token-internal.cc@279
PS7, Line 279: PartitionSchemaPB partition_schema_pb;
> nit: can use *mutable_pb() = std::move(pb) to avoid copying. Same below.
Done
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/scan_token-test.cc@621
PS5, Line 621: ASSERT_OK(tokenWithMetadata->IntoKuduScanner(&scanner_ptr));
> nit: Perhaps we should convert this to some convenience test method like:
Done
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 t
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: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jun 2020 14:27:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/16031/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16031/2//COMMIT_MSG@24
PS2, Line 24: producing
> writing ?
Done
http://gerrit.cloudera.org:8080/#/c/16031/2/src/kudu/client/client.proto
File src/kudu/client/client.proto:
http://gerrit.cloudera.org:8080/#/c/16031/2/src/kudu/client/client.proto@29
PS2, Line 29: optional string table_id = 1;
: optional string table_name = 2;
> If this structure is used only in the context of ScanTokenPB where table_id
The goal of including them here was to encapsulate all the table metadata required to create a KuduTable object client side. It simplifies the semantics around when to use table metadata and when to fallback to calling GetSchema.
I also plan to use this message for more general purpose metadata tokens as tracked in KUDU-3135.
http://gerrit.cloudera.org:8080/#/c/16031/2/src/kudu/client/client.proto@31
PS2, Line 31: required int32 num_replicas = 3;
:
: required SchemaPB schema = 4;
: required PartitionSchemaPB partition_schema = 5;
> If thinking about switching to protobuf3, maybe it's worth turning these in
Done
http://gerrit.cloudera.org:8080/#/c/16031/1/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
http://gerrit.cloudera.org:8080/#/c/16031/1/src/kudu/client/scan_token-internal.cc@117
PS1, Line 117: sp::shared_ptr<KuduTable> table;
> warning: the variable 'metadata' is copy-constructed from a const reference
Done
http://gerrit.cloudera.org:8080/#/c/16031/2/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
http://gerrit.cloudera.org:8080/#/c/16031/2/src/kudu/client/scan_token-internal.cc@119
PS2, Line 119: const TableMetadataPB metadata = message.table_metadata();
> warning: the const qualified variable 'metadata' is copy-constructed from a
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: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 04 Jun 2020 22:41:04 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#13).
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
KUDU-1802: Avoid calls to master when using scan tokens
This patch adds new metadata to the scan token to allow it
to contain all of the metadata required to construct a KuduTable
and open a scanner in the clients. This means the GetTableSchema
and GetTableLocations RPC calls to the master are no longer required
when using the scan token.
New TableMetadataPB, TabletMetadataPB, and authorization token
fields were added as optional fields on the token. Additionally a
`projected_column_idx` field was added that can be used in place
of the `projected_columns`. This significantly reduces the size of
the scan token by not duplicating the ColumnSchemaPB that is
already in the TableMetadataPB.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, tracked
by KUDU-3146, should allow for table metadata to be leveraged in
those cases as well.
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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
I added a test to compare the serialized size of the scan token with
and without the table and tablet metadata. The size results for a
100 column table are:
no metadata: 2697 Bytes
tablet metadata: 2805
tablet, table, and authz metadata: 3258
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
19 files changed, 1,016 insertions(+), 155 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/13
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 13
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 18: Code-Review+2
--
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: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 17:29:12 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jun 2020 18:02:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#18).
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
KUDU-1802: Avoid calls to master when using scan tokens
This patch adds new metadata to the scan token to allow it
to contain all of the metadata required to construct a KuduTable
and open a scanner in the clients. This means the GetTableSchema
and GetTableLocations RPC calls to the master are no longer required
when using the scan token.
New TableMetadataPB, TabletMetadataPB, and authorization token
fields were added as optional fields on the token. Additionally a
`projected_column_idx` field was added that can be used in place
of the `projected_columns`. This significantly reduces the size of
the scan token by not duplicating the ColumnSchemaPB that is
already in the TableMetadataPB.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliency to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, tracked
by KUDU-3146, should allow for table metadata to be leveraged in
those cases as well.
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.
I included the ability to disable this functionality in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforeseen issues or regressions with
this feature.
I added a test to compare the serialized size of the scan token with
and without the table and tablet metadata. The size results for a
100 column table are:
no metadata: 2697 Bytes
tablet metadata: 2805
tablet, table, and authz metadata: 3258
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
19 files changed, 1,018 insertions(+), 132 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/18
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#12).
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
KUDU-1802: Avoid calls to master when using scan tokens
This patch adds new metadata to the scan token to allow it
to contain all of the metadata required to construct a KuduTable
and open a scanner in the clients. This means the GetTableSchema
and GetTableLocations RPC calls to the master are no longer required
when using the scan token.
New TableMetadataPB, TabletMetadataPB, and authorization token
fields were added as optional fields on the token. Additionally a
`projected_column_idx` field was added that can be used in place
of the `projected_columns`. This significantly reduces the size of
the scan token by not duplicating the ColumnSchemaPB that is
already in the TableMetadataPB.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, tracked
by KUDU-3146, should allow for table metadata to be leveraged in
those cases as well.
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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
I added a test to compare the serialized size of the scan token with
and without the table and tablet metadata. The size results for a
100 column table are:
no metadata: 2697 Bytes
tablet metadata: 2805
tablet, table, and authz metadata: 3258
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
18 files changed, 868 insertions(+), 140 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/12
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 12
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#10).
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
KUDU-1802: Avoid call to master when deserializing scan tokens
This patch adds a new table metadata field to the scan token
which contains all of the metadata required to construct a
KuduTable in the clients. This means an RPC call to the
master is no longer required when deserializing the scan token.
The old table name and table id fields are used as a fallback to
maintain backwards compatibility.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, should
allow for table metadata to be leveraged in those cases as well.
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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
13 files changed, 316 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/10
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 5:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/16031/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16031/5//COMMIT_MSG@23
PS5, Line 23: This doesn’t avoid the need for a call to the master in the case
: of writing data to Kudu
It also doesn't populate the meta cache, right? We still need to send GetTableLocationsRequests to the master, no?
http://gerrit.cloudera.org:8080/#/c/16031/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala:
http://gerrit.cloudera.org:8080/#/c/16031/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@60
PS5, Line 60: This shouldn't be required.
nit: it may not be obvious from the laid out solution why this is required. Mind fleshing out the issue a bit more?
http://gerrit.cloudera.org:8080/#/c/16031/5/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:
http://gerrit.cloudera.org:8080/#/c/16031/5/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@232
PS5, Line 232: partitionSchema.getHashBucketSchemas()) {
: Common.PartitionSchemaPB.HashBucketSchemaPB.Builder hbsBuilder =
: Common.PartitionSchemaPB.HashBucketSchemaPB.newBuilder()
: .addAllColumns(idsToPb(hashBucketSchema.getColumnIds()))
: .setNumBuckets(hashBucketSchema.getNumBuckets())
: .setSeed(hashBucketSchema.getSeed());
: builder.addHashBucketSchemas(hbsBuilder.build());
: }
:
: Common.PartitionSchemaPB.RangeSchemaPB rangeSchemaPB =
: Common.PartitionSchemaPB.RangeSchemaPB.newBuilder()
: .addAllColumns(idsToPb(partitionSchema.getRangeSchema().getColumnIds()))
: .build();
nit: typical newline alignment in Java code is to 4 spaces, not 8, though required by the GSG. Also align the .calls()? Same elsewhere
http://gerrit.cloudera.org:8080/#/c/16031/5/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/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@489
PS5, Line 489: // TODO(ghenke): This shouldn't be required. Instead we should use snapshot scans
: // and Kudu should use the schema at the given snapshot time when scanning or we
: // should leverage column ids in the projection and predicates for scans
: // (similar to how we use the table id to handle table renames).
Should this be a jira? Seems high-hanging enough that a TODO might not do this justice, especially if we're copying this rationale everywhere.
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/client.proto
File src/kudu/client/client.proto:
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/client.proto@28
PS5, Line 28: message TableMetadataPB {
nit: add some docs explaining how this can be used, maybe adding some color around what RPCs are sent in the absence of these tokens.
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/scan_token-internal.cc@128
PS5, Line 128: table.reset(new KuduTable(client->shared_from_this(), message.table_name(), message.table_id(),
: metadata.num_replicas(), kuduSchema, partition_schema, extra_configs));
This doesn't clear the meta cache as we would in OpenTable() -- is that important? There's a blurb in client-internal.cc about why it might be.
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/scan_token-internal.cc@266
PS5, Line 266: pb.set_table_id(table->id());
: pb.set_table_name(table->name());
Since these are optional when including table metadata, is there a reason to set these?
Ah from the commit message, it's for backwards compat. Maybe add a comment here denoting that, since it isn't obvious.
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/scan_token-test.cc@621
PS5, Line 621: ASSERT_OK(tokenWithMetadata->IntoKuduScanner(&scanner_ptr));
nit: Perhaps we should convert this to some convenience test method like:
Status IntoUniqueScanner(const KuduScanToken& token, unique_ptr<KuduScanner>* scanner_ptr) {
KuduScanner* scanner_ptr_raw;
RETURN_NOT_OK(token.IntoKuduScanner(&scanner_ptr_raw));
scanner_ptr->reset(scanner_ptr_raw);
return Status::OK()
}
--
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: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 08 Jun 2020 22:23:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16031/3/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/16031/3/src/kudu/client/scan_token-test.cc@621
PS3, Line 621: ASSERT_OK(tokenWithMetadata->IntoKuduScanner(&scanner_ptr));
As we discussed offline, this is a memory leak. It's necessary to release the memory owned by 'scanner_ptr' before calling IntoKuduScanner() again.
--
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: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 05 Jun 2020 20:05:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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 10: Code-Review-1
After talking with Andrew a bit I am going to work on some additional tests and fields for this.
--
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: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jun 2020 19:52:27 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 19: Code-Review+2
Thank you very much for addressing this!
--
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: 19
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 29 Jun 2020 18:04:24 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16031/8/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/16031/8/src/kudu/client/scan_token-test.cc@147
PS8, Line 147: Status IntoUniqueScanner(const KuduScanToken& token, unique_ptr<KuduScanner>* scanner_ptr) {
> warning: method 'IntoUniqueScanner' can be made static [readability-convert
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: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jun 2020 14:35:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#14).
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
KUDU-1802: Avoid calls to master when using scan tokens
This patch adds new metadata to the scan token to allow it
to contain all of the metadata required to construct a KuduTable
and open a scanner in the clients. This means the GetTableSchema
and GetTableLocations RPC calls to the master are no longer required
when using the scan token.
New TableMetadataPB, TabletMetadataPB, and authorization token
fields were added as optional fields on the token. Additionally a
`projected_column_idx` field was added that can be used in place
of the `projected_columns`. This significantly reduces the size of
the scan token by not duplicating the ColumnSchemaPB that is
already in the TableMetadataPB.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, tracked
by KUDU-3146, should allow for table metadata to be leveraged in
those cases as well.
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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
I added a test to compare the serialized size of the scan token with
and without the table and tablet metadata. The size results for a
100 column table are:
no metadata: 2697 Bytes
tablet metadata: 2805
tablet, table, and authz metadata: 3258
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
19 files changed, 1,018 insertions(+), 155 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/14
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 14
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Greg Solovyev (Code Review)" <ge...@cloudera.org>.
Greg Solovyev 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 12:
(1 comment)
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 flag to rule them all in Scala?
--
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: 12
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jun 2020 16:53:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#4).
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
KUDU-1802: Avoid call to master when deserializing scan tokens
This patch adds a new table metadata field to the scan token
which contains all of the metadata required to construct a
KuduTable in the clients. This means an RPC call to the
master is no longer required when deserializing the scan token.
The old table name and table id fields are used as a fallback to
maintain backwards compatibility.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, should
allow for table metadata to be leveraged in those cases as well.
This doesn’t avoid the need for a call to the master 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.
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
10 files changed, 225 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/4
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon 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 3:
One potential concern here -- the list of splits/tokens for a given query/job is O(number of tablets) and may be submitted as part of the job. Here you end up duplicating the table metadata O(n) times instead of just once for the job. Will that be problematic with tables with thousands of tablets, and big schemas? Are we going to hit RPC size limit or task description limit issues?
An alternate to consider is just the ability to ask the client to serialize a "table metadata" token, and broadcast that across the tasks (eg in a spark job) separately from the per-task tokens. Curious if you considered that.
--
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: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 05 Jun 2020 22:48:21 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 11:
The c++ client still needs work to handle injecting the tablet metadata into the meta_cache, but I pushed for now because the Java client and tests are done and what I would emulate in the c++ client/tests.
--
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: 11
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 23 Jun 2020 19:40:05 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#17).
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
KUDU-1802: Avoid calls to master when using scan tokens
This patch adds new metadata to the scan token to allow it
to contain all of the metadata required to construct a KuduTable
and open a scanner in the clients. This means the GetTableSchema
and GetTableLocations RPC calls to the master are no longer required
when using the scan token.
New TableMetadataPB, TabletMetadataPB, and authorization token
fields were added as optional fields on the token. Additionally a
`projected_column_idx` field was added that can be used in place
of the `projected_columns`. This significantly reduces the size of
the scan token by not duplicating the ColumnSchemaPB that is
already in the TableMetadataPB.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliency to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, tracked
by KUDU-3146, should allow for table metadata to be leveraged in
those cases as well.
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.
I included the ability to disable this functionality in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforeseen issues or regressions with
this feature.
I added a test to compare the serialized size of the scan token with
and without the table and tablet metadata. The size results for a
100 column table are:
no metadata: 2697 Bytes
tablet metadata: 2805
tablet, table, and authz metadata: 3258
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
19 files changed, 1,018 insertions(+), 132 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/17
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 17
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jun 2020 22:17:06 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 18: Verified+1
Unrelated test failure in MultiThreadedTabletTest/4.DeleteAndReinsert , due to already known issue https://issues.apache.org/jira/browse/KUDU-2667
--
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: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 17:23:02 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#3).
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
KUDU-1802: Avoid call to master when deserializing scan tokens
This patch adds a new table metadata field to the scan token
which contains all of the metadata required to construct a
KuduTable in the clients. This means an RPC call to the
master is no longer required when deserializing the scan token.
The old table name and table id fields are used as a fallback to
maintain backwards compatibility.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, should
allow for table metadata to be leveraged in those cases as well.
This doesn’t avoid the need for a call to the master 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.
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
10 files changed, 224 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/3
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#8).
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
KUDU-1802: Avoid call to master when deserializing scan tokens
This patch adds a new table metadata field to the scan token
which contains all of the metadata required to construct a
KuduTable in the clients. This means an RPC call to the
master is no longer required when deserializing the scan token.
The old table name and table id fields are used as a fallback to
maintain backwards compatibility.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, should
allow for table metadata to be leveraged in those cases as well.
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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
13 files changed, 314 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/8
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 15:
(9 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
http://gerrit.cloudera.org:8080/#/c/16031/15//COMMIT_MSG@35
PS15, Line 35: functionlity
functionality
http://gerrit.cloudera.org:8080/#/c/16031/15//COMMIT_MSG@37
PS15, Line 37: unforseen
unforeseen
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@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 t
Yes, right -- I meant GetTableLocations RPC populate tablet location information (i.e. tablet metadata in this context). You are right: there isn't GetTableMetadata RPC in Kudu.
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");
> The code path is the same except that this also validates that the authz to
Great. Do we have assurance that the code paths for --tserver_enforce_access_control=false are covered w.r.t. the newly introduced 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)
> Yes, they are independent.
OK, sounds good to me.
Do you think it's worth adding a small test case to verify that a builder with
.includeTabletMetadata(false)
.includeTableMetadata(true)
works as expected?
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto
File src/kudu/client/client.proto:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto@103
PS14, Line 103:
> I don't think we need to. It's schema not data. We don't redact projected_c
Indeed: SGTM.
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: // Cached table locations should not live longer than this timeout.
: optional uint64 ttl_millis = 5;
Ideally, we should have set the expiration date in absolute terms given how the tokens are generated and used: they might spend some time 'on the shelf' by the time they are used. Using TTL means we assume that time interval is short.
Do you think it's worth adding a doc blurb about this and what client does when it finds the metadata is expired?
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@158
PS14, Line 158: entry
> LookupEntryByKeyFastPath will return false if the entry is stale.
Ah, indeed: LookupEntryByKeyFastPath() returns false for stale entries.
--
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: 15
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 02:41:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#11).
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
KUDU-1802: Avoid calls to master when using scan tokens
This patch adds new metadata to the scan token to allow it
to contain all of the metadata required to construct a KuduTable
and open a scanner in the clients. This means the GetTableSchema
and GetTableLocations RPC calls to the master are no longer required
when using the scan token.
New TableMetadataPB, TabletMetadataPB, and authorization token
fields were added as optional fields on the token. Additionally a
`projected_column_idx` field was added that can be used in place
of the `projected_columns`. This significantly reduces the size of
the scan token by not duplicating the ColumnSchemaPB that is
already in the TableMetadataPB.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, tracked
by KUDU-3146, should allow for table metadata to be leveraged in
those cases as well.
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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
I added a test to compare the serialized size of the scan token with
and without the table and tablet metadata. The size results for a
100 column table are:
no metadata: 2697 Bytes
tablet metadata: 2805
tablet, table, and authz metadata: 3258
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
18 files changed, 857 insertions(+), 133 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/11
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 03:04:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
Removed Verified+1 by Kudu Jenkins (120)
--
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: deleteVote
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 20: Verified+1 Code-Review+2
Carrying Alexey's +2 through the automatic rebase.
--
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: 20
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Jun 2020 12:45:41 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 18: Code-Review-1
I am still doing some manual testing.
--
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: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 20:50:31 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 15:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.h@2661
PS14, Line 2661: include_metadata
> include_tablet_metadata ?
Done
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto
File src/kudu/client/client.proto:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto@44
PS14, Line 44: // Metdata about a single server.
: // This can be used on the client to update the local cache of where
: // each server is located.
: message ServerMetadataPB {
: optional bytes uuid = 1;
:
:
> nit: add a doc for this message?
Done
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto@103
PS14, Line 103:
> add [(kudu.REDACT) = true]?
I don't think we need to. It's schema not data. We don't redact projected_columns, which is very similar.
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto@107
PS14, Line 107: optional TableMetadataPB table_metadata = 21;
> ditto
Same as above.
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/meta_cache.h
File src/kudu/client/meta_cache.h:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/meta_cache.h@341
PS14, Line 341: const MonoTime& expiration_time() const {
> add 'const' specifier?
Done
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@158
PS14, Line 158: entry
> Does it make sense to check for the staleness of an entry and populate the
LookupEntryByKeyFastPath will return false if the entry is stale.
Good catch. I do put the ttl on the metadata and propagate it in the java client. I meant to do it here too.
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@206
PS14, Line 206: e
> no need to have reference for int, I think.
Done
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@210
PS14, Line 210:
> style int: stick '&' to the type
Done
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@210
PS14, Line 210: {
> nit: replace with
I like to see the type coming from the protobuf messages. Given it's generated code, I find it can make finding usages via grep easier too.
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-test.cc@785
PS14, Line 785: aut
> nit here and below: auto
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: 15
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 01:09:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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 3:
The tokens do need to fit in memory so adding more metadata would have overhead there. Though larger table schemas and more parallel tasks generally require more memory. I think that is somewhat expected. I will add a config to Spark that would allow for table metadata on tokens to be disabled in case that is an issue.
--
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: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 06 Jun 2020 00:23:50 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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 10: Code-Review+2
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16031/10/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/10/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@200
PS10, Line 200: tableMetadata.getSchema()
nit: since those fields are option in proto file, maybe it's worth adding a sanity check Preconditions.checkNotNull or alike?
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: table_pb.mutable_partition_schema()->CopyFrom(partition_schema_pb);
nit: use std::move() here as well?
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: int init_requests = METRIC_handler_latency_kudu_master_MasterService_GetTableSchema
nit: add 'const' ?
http://gerrit.cloudera.org:8080/#/c/16031/10/src/kudu/client/scan_token-test.cc@764
PS10, Line 764: METRIC_handler_latency_kudu_master_MasterService_GetTableSchema
: .Instantiate(ent)
nit: for better readability, maybe introduce a variable for this (a const pointer?) and use it these 3 places?
--
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: 10
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jun 2020 19:22:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16031 )
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
KUDU-1802: Avoid calls to master when using scan tokens
This patch adds new metadata to the scan token to allow it
to contain all of the metadata required to construct a KuduTable
and open a scanner in the clients. This means the GetTableSchema
and GetTableLocations RPC calls to the master are no longer required
when using the scan token.
New TableMetadataPB, TabletMetadataPB, and authorization token
fields were added as optional fields on the token. Additionally a
`projected_column_idx` field was added that can be used in place
of the `projected_columns`. This significantly reduces the size of
the scan token by not duplicating the ColumnSchemaPB that is
already in the TableMetadataPB.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliency to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, tracked
by KUDU-3146, should allow for table metadata to be leveraged in
those cases as well.
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.
I included the ability to disable this functionality in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforeseen issues or regressions with
this feature.
I added a test to compare the serialized size of the scan token with
and without the table and tablet metadata. The size results for a
100 column table are:
no metadata: 2697 Bytes
tablet metadata: 2805
tablet, table, and authz metadata: 3258
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Reviewed-on: http://gerrit.cloudera.org:8080/16031
Reviewed-by: Grant Henke <gr...@apache.org>
Tested-by: Grant Henke <gr...@apache.org>
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
19 files changed, 1,024 insertions(+), 136 deletions(-)
Approvals:
Grant Henke: Looks good to me, approved; Verified
--
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: merged
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 21
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#15).
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
KUDU-1802: Avoid calls to master when using scan tokens
This patch adds new metadata to the scan token to allow it
to contain all of the metadata required to construct a KuduTable
and open a scanner in the clients. This means the GetTableSchema
and GetTableLocations RPC calls to the master are no longer required
when using the scan token.
New TableMetadataPB, TabletMetadataPB, and authorization token
fields were added as optional fields on the token. Additionally a
`projected_column_idx` field was added that can be used in place
of the `projected_columns`. This significantly reduces the size of
the scan token by not duplicating the ColumnSchemaPB that is
already in the TableMetadataPB.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, tracked
by KUDU-3146, should allow for table metadata to be leveraged in
those cases as well.
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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
I added a test to compare the serialized size of the scan token with
and without the table and tablet metadata. The size results for a
100 column table are:
no metadata: 2697 Bytes
tablet metadata: 2805
tablet, table, and authz metadata: 3258
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
19 files changed, 1,004 insertions(+), 136 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/15
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 15
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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 18:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16031/18/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/18/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@500
PS18, Line 500: // Even if we add the table metadata, we should also set the old table id
: // and name for backward compatibility.
: proto.setTableId(table.getTableId());
: proto.setTableName(table.getName());
> Good catch! Did you find that by testing against real some system? Do you
I didn't see any specific issue yet. I wen't back and forth with it on this patch, but decided to add it just in case.
I am working on testing with Impala right now.
--
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: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 17:32:46 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#2).
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
KUDU-1802: Avoid call to master when deserializing scan tokens
This patch adds a new table metadata field to the scan token
which contains all of the metadata required to construct a
KuduTable in the clients. This means an RPC call to the
master is no longer required when deserializing the scan token.
The old table name and table id fields are used as a fallback to
maintain backwards compatibility.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, should
allow for table metadata to be leveraged in those cases as well.
This doesn’t avoid the need for a call to the master in the case
of producing data to Kudu, that work is tracked by KUDU-3135.
I expect the TableMetadataPB message would be used there as
well.
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
10 files changed, 224 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/2
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jun 2020 04:44:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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 4: Verified+1
--
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: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 08 Jun 2020 19:52:55 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#5).
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
KUDU-1802: Avoid call to master when deserializing scan tokens
This patch adds a new table metadata field to the scan token
which contains all of the metadata required to construct a
KuduTable in the clients. This means an RPC call to the
master is no longer required when deserializing the scan token.
The old table name and table id fields are used as a fallback to
maintain backwards compatibility.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, should
allow for table metadata to be leveraged in those cases as well.
This doesn’t avoid the need for a call to the master 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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
13 files changed, 238 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/5
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has removed a vote on this change.
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
Removed Verified+1 by Alexey Serbin <as...@cloudera.com>
--
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: deleteVote
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#7).
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
KUDU-1802: Avoid call to master when deserializing scan tokens
This patch adds a new table metadata field to the scan token
which contains all of the metadata required to construct a
KuduTable in the clients. This means an RPC call to the
master is no longer required when deserializing the scan token.
The old table name and table id fields are used as a fallback to
maintain backwards compatibility.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, should
allow for table metadata to be leveraged in those cases as well.
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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
13 files changed, 241 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/7
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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 5:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/16031/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16031/5//COMMIT_MSG@23
PS5, Line 23: This doesn’t avoid the need for a call to the master in the case
: of writing data to Kudu
> It also doesn't populate the meta cache, right? We still need to send GetTa
Right, this is specifically avoiding the schema metadata currently.
http://gerrit.cloudera.org:8080/#/c/16031/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
File java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala:
http://gerrit.cloudera.org:8080/#/c/16031/5/java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala@60
PS5, Line 60: This shouldn't be required.
> nit: it may not be obvious from the laid out solution why this is required.
Done
http://gerrit.cloudera.org:8080/#/c/16031/5/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
File java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java:
http://gerrit.cloudera.org:8080/#/c/16031/5/java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java@232
PS5, Line 232: partitionSchema.getHashBucketSchemas()) {
: Common.PartitionSchemaPB.HashBucketSchemaPB.Builder hbsBuilder =
: Common.PartitionSchemaPB.HashBucketSchemaPB.newBuilder()
: .addAllColumns(idsToPb(hashBucketSchema.getColumnIds()))
: .setNumBuckets(hashBucketSchema.getNumBuckets())
: .setSeed(hashBucketSchema.getSeed());
: builder.addHashBucketSchemas(hbsBuilder.build());
: }
:
: Common.PartitionSchemaPB.RangeSchemaPB rangeSchemaPB =
: Common.PartitionSchemaPB.RangeSchemaPB.newBuilder()
: .addAllColumns(idsToPb(partitionSchema.getRangeSchema().getColumnIds()))
: .build();
> nit: typical newline alignment in Java code is to 4 spaces, not 8, though r
Done
http://gerrit.cloudera.org:8080/#/c/16031/5/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/5/java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java@489
PS5, Line 489: // TODO(ghenke): This shouldn't be required. Instead we should use snapshot scans
: // and Kudu should use the schema at the given snapshot time when scanning or we
: // should leverage column ids in the projection and predicates for scans
: // (similar to how we use the table id to handle table renames).
> Should this be a jira? Seems high-hanging enough that a TODO might not do t
yeah, definitely. I meant to file one, thanks for pointing it out.
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/client.proto
File src/kudu/client/client.proto:
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/client.proto@28
PS5, Line 28: message TableMetadataPB {
> nit: add some docs explaining how this can be used, maybe adding some color
I add the color below in the scan token given this can be used more generically for future use too. I will add some context here though.
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/scan_token-internal.cc@128
PS5, Line 128: table.reset(new KuduTable(client->shared_from_this(), message.table_name(), message.table_id(),
: metadata.num_replicas(), kuduSchema, partition_schema, extra_configs));
> This doesn't clear the meta cache as we would in OpenTable() -- is that imp
This comment is also on `AsyncKuduClient.getTableSchema`.
That comment was added in https://github.com/apache/kudu/commit/c1f78ffb59af28c0f453cb8485ed946465f19f43 and looks to specifically address issues around newly created tables cached state. I don't think it is an issue for scan tokens.
http://gerrit.cloudera.org:8080/#/c/16031/5/src/kudu/client/scan_token-internal.cc@266
PS5, Line 266: pb.set_table_id(table->id());
: pb.set_table_name(table->name());
> Since these are optional when including table metadata, is there a reason t
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: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 09 Jun 2020 03:41:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 18:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16031/18/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/18/java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java@500
PS18, Line 500: // Even if we add the table metadata, we should also set the old table id
: // and name for backward compatibility.
: proto.setTableId(table.getTableId());
: proto.setTableName(table.getName());
Good catch! Did you find that by testing against real some system? Do you think it's easy to add some test case to catch this if there were an regression?
--
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: 18
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 17:28:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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: Verified+1
--
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jun 2020 19:00:10 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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 3:
> Are we going to hit RPC size limit or task description limit issues?
Today in Spark we use 1 scan token per task. So this shouldn't result in multiples being sent. It moves any potential large schema throughput "problem" from the `master -> task` to the `driver /coordinator-> task`. Do you know if Impala behaves differently then Spark?
> An alternate to consider is just the ability to ask the client to serialize a "table metadata" token, and broadcast that across the tasks (eg in a spark job) separately from the per-task tokens.
I had considered this, and plan to potentially leverage something like this for jobs that write data to Kudu (KUDU-3135). The benefit of this approach is that it works today with no changes to applications/frameworks already using scan tokens. Given there is a 1-1 relationship between tasks and scan tokens in spark, it felt more natural to include the schema in the token.
--
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: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 05 Jun 2020 23:16:05 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
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:
I addressed the C++ client TODOs. This is ready for review.
--
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jun 2020 18:02:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.h
File src/kudu/client/client.h:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.h@2661
PS14, Line 2661: include_table_metadata
include_tablet_metadata ?
or maybe just replace with
include_metadata
for both methods?
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto
File src/kudu/client/client.proto:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto@44
PS14, Line 44: message ServerMetadataPB {
: optional bytes uuid = 1;
:
: repeated HostPortPB rpc_addresses = 2;
:
: optional string location = 3;
: }
nit: add a doc for this message?
Also, maybe it's worth documenting what 'location' means?
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto@103
PS14, Line 103: optional TableMetadataPB table_metadata = 21;
add [(kudu.REDACT) = true]?
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/client.proto@107
PS14, Line 107: optional TabletMetadataPB tablet_metadata = 22;
ditto
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/meta_cache.h
File src/kudu/client/meta_cache.h:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/meta_cache.h@341
PS14, Line 341: const MonoTime& expiration_time() {
add 'const' specifier?
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@158
PS14, Line 158: entry
Does it make sense to check for the staleness of an entry and populate the location information from the token, not from the cache in case if a stale entry is found?
This also prompts a question: do we need to put TTL of the the location info into scan tokens?
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@206
PS14, Line 206: &
no need to have reference for int, I think.
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@210
PS14, Line 210: &
style int: stick '&' to the type
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-internal.cc@210
PS14, Line 210: const ColumnSchemaPB &column
nit: replace with
const auto& column
?
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/16031/14/src/kudu/client/scan_token-test.cc@785
PS14, Line 785: int
nit here and below: auto
otherwise I expect compiler would warn about narrowing from 'uint64_t' to 'int' and signed/unsigned?
--
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jun 2020 23:03:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Greg Solovyev, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#19).
Change subject: KUDU-1802: Avoid calls to master when using scan tokens
......................................................................
KUDU-1802: Avoid calls to master when using scan tokens
This patch adds new metadata to the scan token to allow it
to contain all of the metadata required to construct a KuduTable
and open a scanner in the clients. This means the GetTableSchema
and GetTableLocations RPC calls to the master are no longer required
when using the scan token.
New TableMetadataPB, TabletMetadataPB, and authorization token
fields were added as optional fields on the token. Additionally a
`projected_column_idx` field was added that can be used in place
of the `projected_columns`. This significantly reduces the size of
the scan token by not duplicating the ColumnSchemaPB that is
already in the TableMetadataPB.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliency to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, tracked
by KUDU-3146, should allow for table metadata to be leveraged in
those cases as well.
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.
I included the ability to disable this functionality in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforeseen issues or regressions with
this feature.
I added a test to compare the serialized size of the scan token with
and without the table and tablet metadata. The size results for a
100 column table are:
no metadata: 2697 Bytes
tablet metadata: 2805
tablet, table, and authz metadata: 3258
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/main/java/org/apache/kudu/client/RemoteTablet.java
M java/kudu-client/src/main/java/org/apache/kudu/client/TableLocationsCache.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/CMakeLists.txt
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/meta_cache.cc
M src/kudu/client/meta_cache.h
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
19 files changed, 1,024 insertions(+), 136 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/19
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 19
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#6).
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
KUDU-1802: Avoid call to master when deserializing scan tokens
This patch adds a new table metadata field to the scan token
which contains all of the metadata required to construct a
KuduTable in the clients. This means an RPC call to the
master is no longer required when deserializing the scan token.
The old table name and table id fields are used as a fallback to
maintain backwards compatibility.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, should
allow for table metadata to be leveraged in those cases as well.
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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
13 files changed, 241 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/6
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/16031
to look at the new patch set (#9).
Change subject: KUDU-1802: Avoid call to master when deserializing scan tokens
......................................................................
KUDU-1802: Avoid call to master when deserializing scan tokens
This patch adds a new table metadata field to the scan token
which contains all of the metadata required to construct a
KuduTable in the clients. This means an RPC call to the
master is no longer required when deserializing the scan token.
The old table name and table id fields are used as a fallback to
maintain backwards compatibility.
Adding the table metadata to the scan token is enabled by
default given it’s more scalable and performant. However,
it can be disabled in rare cases where more resiliancy to
column renaming is desired. One example where disabling the
table metadata is used is the backup job. Future work, should
allow for table metadata to be leveraged in those cases as well.
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.
I included the ability to disable this functionlity in the
kudu-spark integration via `kudu.useDriverMetadata` just
in case there are any unforseen issues or regressions with
this feature.
Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
---
M java/kudu-backup/src/main/scala/org/apache/kudu/backup/KuduBackupRDD.scala
M java/kudu-client/src/main/java/org/apache/kudu/client/KuduScanToken.java
M java/kudu-client/src/main/java/org/apache/kudu/client/ProtobufHelper.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestScanToken.java
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduRDD.scala
M java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduReadOptions.scala
M src/kudu/client/client.cc
M src/kudu/client/client.h
M src/kudu/client/client.proto
M src/kudu/client/scan_token-internal.cc
M src/kudu/client/scan_token-internal.h
M src/kudu/client/scan_token-test.cc
13 files changed, 315 insertions(+), 35 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/31/16031/9
--
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: newpatchset
Gerrit-Change-Id: I88c1b8392de37dd5e8b7bd8b78a21603ff8b1d1b
Gerrit-Change-Number: 16031
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke 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 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16031/3/src/kudu/client/scan_token-test.cc
File src/kudu/client/scan_token-test.cc:
http://gerrit.cloudera.org:8080/#/c/16031/3/src/kudu/client/scan_token-test.cc@621
PS3, Line 621: ASSERT_OK(tokenWithMetadata->IntoKuduScanner(&scanner_ptr));
> As we discussed offline, this is a memory leak. It's necessary to release
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: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 08 Jun 2020 16:34:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Greg Solovyev (Code Review)" <ge...@cloudera.org>.
Greg Solovyev 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: Code-Review+1
--
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 <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jun 2020 19:14:44 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-1802: Avoid calls to master when using scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 16: Code-Review+2
(3 comments)
There is a report on an issue in LINT build. Otherwise, LGTM.
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");
> I am not sure what you mean. The only difference I am aware of is the clien
Right: I just wanted to make sure we have the coverage for both cases where --tserver_enforce_access_control=false and --tserver_enforce_access_control=true for the new code. In particular, I was concerned that there might be some unexpected behavior if the new code in case if --tserver_enforce_access_control=false. From the code I can see it's not the case, but if looking at that as a black box, it would be nice to have the tests run against that for --tserver_enforce_access_control=false. I guess the latter is somehow covered by other tests where scan tokens are involved.
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)
> Sure I can add it to the size test.
Thank you!
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: // Cached table locations should not live longer than this timeout.
: // Note: Some time will pass be
> I will add a docs blurb.
Yes, all these TTL and point-in-time issues are hard to be really precise with, but I think there isn't a need for precise timings, but it's better to explicitly mention that.
Thank you for adding the comment!
--
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: 16
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Jun 2020 03:40:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1802: Avoid call to master when deserializing scan tokens
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin 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 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/16031/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/16031/2//COMMIT_MSG@24
PS2, Line 24: producing
writing ?
http://gerrit.cloudera.org:8080/#/c/16031/2/src/kudu/client/client.proto
File src/kudu/client/client.proto:
http://gerrit.cloudera.org:8080/#/c/16031/2/src/kudu/client/client.proto@29
PS2, Line 29: optional string table_id = 1;
: optional string table_name = 2;
If this structure is used only in the context of ScanTokenPB where table_id and/or table_name are already present, maybe these fields are not needed?
Also, if getting rid of table_id/table_name and given the fact it's used only as a part of ScanTokenPB, maybe add the rest of the fields as they are into the ScanTokenPB message?
http://gerrit.cloudera.org:8080/#/c/16031/2/src/kudu/client/client.proto@31
PS2, Line 31: required int32 num_replicas = 3;
:
: required SchemaPB schema = 4;
: required PartitionSchemaPB partition_schema = 5;
If thinking about switching to protobuf3, maybe it's worth turning these into optional fields as well?
http://gerrit.cloudera.org:8080/#/c/16031/2/src/kudu/client/scan_token-internal.cc
File src/kudu/client/scan_token-internal.cc:
http://gerrit.cloudera.org:8080/#/c/16031/2/src/kudu/client/scan_token-internal.cc@119
PS2, Line 119: TableMetadataPB
+1
Consider using const reference here instead of copying.
--
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: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 04 Jun 2020 22:27:48 +0000
Gerrit-HasComments: Yes