Only use updated schema if auto update is enabled#26752
Only use updated schema if auto update is enabled#26752reuvenlax merged 1 commit intoapache:masterfrom
Conversation
|
Assigning reviewers. If you would like to opt out of this review, comment R: @kennknowles for label java. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
2b6dda4 to
11de442
Compare
|
Run Java_GCP_IO_Direct PreCommit |
| @@ -691,7 +691,7 @@ void postFlush() { | |||
| @Nullable | |||
| TableSchema updatedTableSchema = | |||
There was a problem hiding this comment.
wait, we actually can only request for updatedTableSchema if autoUpdateSchema, that is include
TableSchema updatedTableSchema =
(streamAppendClient != null) ? streamAppendClient.getUpdatedSchema() : null;
if (updatedTableSchema != null) {
invalidateWriteStream();
appendClientInfo =
Preconditions.checkStateNotNull(getAppendClientInfo(false, updatedTableSchema));
}
in an if block.
There was a problem hiding this comment.
Thanks!
This would make it clearer, but not needed here for now -- streamAppendClient.getUpdatedSchema() does not make external calls, it's pulling a reference that already exists, so there's no practical difference for changing this.
We'll be addressing the bug when the update has to actually happen as part of another PR, and we can consider this path there. WDYT?
There was a problem hiding this comment.
Possibly refer to StorageApiWritesShardedRecords, beside this code path, it used a if clause in the outside:
would be nice to keep Sharded/Unsharded have similar logic.
there are some other code path involve autoUpdateSchema, while this class member is completely unused in StorageApiWritesUnShardedRecords
Updating the schema on-the-fly may cause problems and we need to improve.
This change avoids that problem in case autoSchemaUpdate is disabled, as it shouldn't be considering new schemas anyways.