Hive: Unwrap RuntimeException for Hive TException with rename table

[nk1506]

No description provided.

[pvary]

How widely is alterTable used?
Wouldn't it be better (more general solution) to expose the wrapped MetaException instead?

[nk1506]

It's only being used at two places:

1. To update the table .
2. To rename the table .

Are you proposing to throw non RTE MetaException at alter table . something like

catch (RuntimeException e) {
      if (e.getCause() instanceof InvalidOperationException
          && e.getCause().getMessage() != null
          && e.getCause()
              .getMessage()
              .contains(
                  String.format(
                      "new table %s.%s already exists", table.getDbName(), table.getTableName()))) {
        throw new MetaException(
            String.format("Table already exists: %s.%s", table.getDbName(), table.getTableName()));
      }
      throw e;
    }

[pvary]

I proposing something like this:

catch (RuntimeException e) {
      if (e.getCause() instanceof MetaException) {
        throw (MetaException) e.getCause()
      } else {
        throw e;
      }
    }

[nk1506]

Any thoughts on the test case testRenameTableDestinationTableAlreadyExists since it expects error message as

.isInstanceOf(AlreadyExistsException.class)
        .hasMessageContaining("Table already exists");

Currently we have override this as

.isInstanceOf(RuntimeException.class)
        .hasMessageContaining("new table newdb.table_renamed already exists");

[pvary]

alterTable should return the MetaException and where we call it, we should handle the InvalidOperationException with the message of new table %s.%s already exists and translate it to an AlreadyExistsException.

We should only consider moving the exception if we find that all of the places where we call alterTable we would like to throw the same AlreadyExistsException

[nk1506]

InvalidOperationException still will be wrapped with RuntimeException . So basically we will have to catch RuntimeException and unwrap with AlreadyExistsException .

Or Am I missing something here ?

[nk1506]

Why not unwrap RTE with both MetaException as well as InvalidOperationException and translate to org.apache.hadoop.hive.metastore.api.AlreadyExistsException . Since all the callers of alterTable is already handling org.apache.hadoop.hive.metastore.api.AlreadyExistsException.
I am thinking something like this:

catch (RuntimeException e) {
      // MetaException would be wrapped into RuntimeException during reflection
      if (e.getCause() instanceof MetaException) {
        throw (MetaException) e.getCause();
      }
      // InvalidOperationException would be wrapped into RuntimeException during reflection, as
      // java.lang.RuntimeException:InvalidOperationException(message:new table <> already exists)
      if (e.getCause() instanceof InvalidOperationException
              && e.getCause().getMessage() != null
              && e.getCause()
              .getMessage()
              .contains(
                      String.format(
                              "new table %s.%s already exists", table.getDbName(), table.getTableName()))) {
        throw new AlreadyExistsException(
                String.format("Table already exists: %s.%s", table.getDbName(), table.getTableName()));
      }
      throw e;
    }

[pvary]

In the HiveTableOperations we are accustomed to handle Hive MetaExceptions, so I think handling InvalidOperationException there would be "natural".

The alterTable wraps the exceptions to a RuntimeException because of the DynMethod calls. I think this is a mistake. We should behave the same way as the metaClients.run(client -> client...) like calls, we should throw the original MetaException - my opinion here could be changes if this becomes a breaking change somewhere, but if it does not break code for someone, then we should change it.

[pvary]

I made a mistake in the above command, I thought, that the root exception is MetaException, but in reality it is TException.

So fixing this issue, my comment correctly is below:

---

The alterTable wraps the exceptions to a RuntimeException because of the DynMethod calls. I think this is a mistake. We should behave the same way as the metaClients.run(client -> client...) like calls, we should throw the original TException - my opinion here could be changes if this becomes a breaking change somewhere, but if it does not break code for someone, then we should change it.

[ajantha-bhat]

You should catch here MetaException instead of RuntimeException from the altertable and throw AlreadyExistsException ?

[ajantha-bhat]

or if it is not coming as MetaException, if it is coming as InvalidOperationException cause. We should unwrap this at alterTable and throw only the cause.

[ajantha-bhat]

Unwrapping only MetaException looks odd.

[nk1506]

I am not sure why we should explicitly catch MetaException here. Since it's already being handled with

catch (TException e) {
      throw new RuntimeException("Failed to rename " + from + " to " + to, e);

    }

InvalidOperationException will always be wrapped with RuntimeException in case of alterTable. Either we should unwrap it MetastoreUtil or at every caller of it.

[ajantha-bhat]

Originally we were unwrapping to MetaException but here were still catching RuntimeException. Hence I gave that comment.

Now we are unwrapping to TException and catching the TException. So, now LGTM.

[pvary]

@ajantha-bhat: I think this looks good. Any more comments on your side?

[ajantha-bhat]

LGTM aswell.

[ajantha-bhat]

PR title may need update as it is TException now.

[nk1506]

@pvary , Should we merge this if there are no more comments ?

[pvary]

could we please move this under a catch (InvalidOperationException)?
But keep the old TException handler part

[pvary]

Thanks @nk1506 for the PR!