Hive: Fix metadata file not found#10069
Conversation
Hive: Check e.getMessage() is not null
fix spotlessJavaCheck
lurnagao-dahua
changed the title
| } catch (Throwable e) { | ||
| if (e.getMessage() | ||
| .contains( | ||
| if (e.getMessage() != null |
There was a problem hiding this comment.
It'd better to create a util method for this pattern.
There was a problem hiding this comment.
Where would we use this new method?
If we would use it only in this class, and only in 2 places, then I think it is better not to "hide" what we do behind a method somewhere else in the code.
There was a problem hiding this comment.
If we had the method in the first place, I don't think we would miss the null check here. With this method, we can ensure the null check will be applied in any other places. Just my two cents, which should not block merging this PR.
pvary
left a comment
pvary
left a comment
There was a problem hiding this comment.
Oh... accidentally chose "Approve".
While I think the PR is good, I wanted to wait @manuzhang's review before moving forward.
|
@lurnagao-dahua please check styles.
Is it possible to add a UT for this case? |
Hi, I have checked the style. I found the original pr similar to this one 701, then I will try to write an UT |
nastra
left a comment
nastra
left a comment
There was a problem hiding this comment.
LGTM, it seems we're using the same pattern in a few other places, but having a test to reproduce the underlying issue would be great
|
Thank you for your response! I have been thinking for a while and I have some doubts about this UT. Can you give me some advice? |
|
@lurnagao-dahua: If you check https://github.com/apache/iceberg/blob/3caa3a28d07a2d08b9a0e4196634126f1e016d6a/hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java, you can find plenty of examples for commit errors. Maybe if we could do something similar, like throwing an exception without a message. It would be nice to have a test. OTOH, if the test is more than 50 lines, it would cost us more in the upkeep of the test in the long run, than what we gain with testing a null check. In this case I would skip addig the extra code, following the example of #701. |
Hi,Thank you for your suggestion |
| } | ||
|
|
||
| @Test | ||
| public void testCommitExceptionWithoutMessage() throws TException, InterruptedException { |
There was a problem hiding this comment.
can be simplified to
@Test
public void testCommitExceptionWithoutMessage() throws TException, InterruptedException {
Table table = catalog.loadTable(TABLE_IDENTIFIER);
HiveTableOperations ops = (HiveTableOperations) ((HasTableOperations) table).operations();
TableMetadata metadataV1 = ops.current();
table.updateSchema().addColumn("n", Types.IntegerType.get()).commit();
ops.refresh();
HiveTableOperations spyOps = spy(ops);
doThrow(new RuntimeException()).when(spyOps).persistTable(any(), anyBoolean(), any());
assertThatThrownBy(() -> spyOps.commit(ops.current(), metadataV1))
.isInstanceOf(CommitStateUnknownException.class)
.hasMessageStartingWith("null\nCannot determine whether the commit was successful or not");
}
There was a problem hiding this comment.
@test
public void testCommitExceptionWithoutMessage() throws TException, InterruptedException {
Table table = catalog.loadTable(TABLE_IDENTIFIER);
HiveTableOperations ops = (HiveTableOperations) ((HasTableOperations) table).operations();TableMetadata metadataV1 = ops.current(); table.updateSchema().addColumn("n", Types.IntegerType.get()).commit(); ops.refresh(); HiveTableOperations spyOps = spy(ops); doThrow(new RuntimeException()).when(spyOps).persistTable(any(), anyBoolean(), any()); assertThatThrownBy(() -> spyOps.commit(ops.current(), metadataV1)) .isInstanceOf(CommitStateUnknownException.class) .hasMessageStartingWith("null\nCannot determine whether the commit was successful or not");}
Thank you very much, I have simplified unit test.
4 participants
Hi, I get an Metadata not found in metadata location for table error when trying to query the data generated by flink
The reason is that in some cases, the e.getMessage() return null and it will throw NullPointerException, then skip checkCommitStatus, it may be delete metadataLocation, actually, metadata commit succeed.
It was introduced by 6570