BaseMetastoreTableOperations shouldn't disable refresh upon NoSuchTableException

[stevenzwu]

Otherwise it might cause unfriendly NPE for callers that don't check null return value from current() method.

TableOperations#current() can return null TableMetadata. E.g. BaseMetastoreTableOperations returns null when encountered NoSuchTableException.

  @Override
  public TableMetadata current() {
    if (shouldRefresh) {
      return refresh();
    }
    return currentMetadata;
  }

  @Override
  public TableMetadata refresh() {
    boolean currentMetadataWasAvailable = currentMetadata != null;
    try {
      doRefresh();
    } catch (NoSuchTableException e) {
      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;
      throw e;
    }
    return current();
  }

Callers handle the null in inconsistent way. Should TableOperations#current() just throw more friendly NoSuchTableException or a new TableNotLoadedException instead? At least, this ensures a more consistent handling and informative exception.

Maybe a few callers do the null check, like BaseMetastoreCatalog. Most callers (like MergingSnapshotProducer) don't check and can throw unfriendly NPE in this case.

java.lang.NullPointerException
        at org.apache.iceberg.MergingSnapshotProducer.<init>(MergingSnapshotProducer.java:81)
        at org.apache.iceberg.MergeAppend.<init>(MergeAppend.java:32)
        at org.apache.iceberg.BaseTransaction.newAppend(BaseTransaction.java:124)

[SteNicholas]

@stevenzwu I agree with you for the point that the return behavior of TableOperations#current() should be unified, which could be confused for users when TableOperations#current() returns null.

[rdblue]

current will return null only after refreshing the table throws NoSuchTableException. I think this is minor because callers would have ignored that exception to fail when current returns null.

That said, I think we can throw the exception and set shouldRefresh = true. That way, any call to get table metadata will trigger a refresh and either get the table back to a consistent state, or fail with the same exception.

[stevenzwu]

@rdblue your suggestion makes sense to me. I created a PR for that: #1553