You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/10/06 17:06:39 UTC

[GitHub] [iceberg] rdblue commented on a change in pull request #1553: BaseMetastoreTableOperations shouldn't disable refresh upon NoSuchTab…

rdblue commented on a change in pull request #1553:
URL: https://github.com/apache/iceberg/pull/1553#discussion_r500434383



##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java
##########
@@ -88,11 +88,9 @@ public TableMetadata refresh() {
       if (currentMetadataWasAvailable) {
         LOG.warn("Could not find the table during refresh, setting current metadata to null", e);
       }
-

Review comment:
       Can you add this line back? Removing it will just cause commit conflicts and it isn't needed for the PR.

##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java
##########
@@ -88,11 +88,9 @@ public TableMetadata refresh() {
       if (currentMetadataWasAvailable) {
         LOG.warn("Could not find the table during refresh, setting current metadata to null", e);
       }
-
       currentMetadata = null;
       currentMetadataLocation = null;
       version = -1;
-      shouldRefresh = false;

Review comment:
       I think this needs to set `shouldRefresh` to true. If `refresh` was called directly, then `shouldRefresh` may not already be true. But we want it to be true so that this will not return metadata until a successful refresh.

##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreTableOperations.java
##########
@@ -88,11 +88,9 @@ public TableMetadata refresh() {
       if (currentMetadataWasAvailable) {
         LOG.warn("Could not find the table during refresh, setting current metadata to null", e);
       }
-
       currentMetadata = null;
       currentMetadataLocation = null;
       version = -1;
-      shouldRefresh = false;

Review comment:
       Also, since `current` is called in `commit`, I think we will need to only set this in the case when `currentMetadataWasAvailable`. That way, creating a table will still work the same way.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org