You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ConeyLiu (via GitHub)" <gi...@apache.org> on 2023/06/28 09:33:34 UTC
[GitHub] [iceberg] ConeyLiu opened a new pull request, #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
ConeyLiu opened a new pull request, #7931:
URL: https://github.com/apache/iceberg/pull/7931
We should set the `commitStatus` to `UNKNOWN` before throwing the `CommitStateUnknownException`. Otherwise the `commitStatus` is still `FAILURE` which could lead to the metadata being deleted in `cleanupMetadataAndUnlock`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] ConeyLiu commented on a diff in pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "ConeyLiu (via GitHub)" <gi...@apache.org>.
ConeyLiu commented on code in PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#discussion_r1246161729
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -352,6 +352,49 @@ public void testNoLockThriftExceptionConcurrentCommit() throws TException, Inter
Assert.assertEquals("New metadata files should not exist", 2, metadataFileCount(ops.current()));
}
+ @Test
+ public void testLockExceptionUnknownSuccessCommit() 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();
+
+ TableMetadata metadataV2 = ops.current();
+
+ Assertions.assertThat(ops.current().schema().columns().size()).isEqualTo(2);
+
+ HiveTableOperations spyOps = spy(ops);
+
+ // Simulate a communication error after a successful commit
+ doAnswer(
+ i -> {
+ org.apache.hadoop.hive.metastore.api.Table tbl =
+ i.getArgument(0, org.apache.hadoop.hive.metastore.api.Table.class);
+ String location = i.getArgument(2, String.class);
+ ops.persistTable(tbl, true, location);
+ throw new LockException("Datacenter on fire");
+ })
+ .when(spyOps)
+ .persistTable(any(), anyBoolean(), any());
+
+ Assertions.assertThatThrownBy(() -> spyOps.commit(metadataV2, metadataV1))
Review Comment:
Hi @pvary, this UT is adapted from the above UT and here we commit the `metadataV2` back to `metadataV1`. And we throw the `LockException` after the commit succeeded. So the following should be `Current metadata should have changed`.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#discussion_r1249086717
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -352,6 +352,49 @@ public void testNoLockThriftExceptionConcurrentCommit() throws TException, Inter
Assert.assertEquals("New metadata files should not exist", 2, metadataFileCount(ops.current()));
}
+ @Test
+ public void testLockExceptionUnknownSuccessCommit() 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();
+
+ TableMetadata metadataV2 = ops.current();
+
+ Assertions.assertThat(ops.current().schema().columns().size()).isEqualTo(2);
+
+ HiveTableOperations spyOps = spy(ops);
+
+ // Simulate a communication error after a successful commit
+ doAnswer(
+ i -> {
+ org.apache.hadoop.hive.metastore.api.Table tbl =
+ i.getArgument(0, org.apache.hadoop.hive.metastore.api.Table.class);
+ String location = i.getArgument(2, String.class);
+ ops.persistTable(tbl, true, location);
+ throw new LockException("Datacenter on fire");
+ })
+ .when(spyOps)
+ .persistTable(any(), anyBoolean(), any());
+
+ Assertions.assertThatThrownBy(() -> spyOps.commit(metadataV2, metadataV1))
+ .hasMessageContaining("Failed to heartbeat for hive lock while")
+ .isInstanceOf(CommitStateUnknownException.class);
+
+ ops.refresh();
+
+ Assertions.assertThat(ops.current())
+ .as("Current metadata should have changed")
Review Comment:
Hmm, for now could we just check locations? Then we can do a full equal implementation in another or
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -352,6 +352,49 @@ public void testNoLockThriftExceptionConcurrentCommit() throws TException, Inter
Assert.assertEquals("New metadata files should not exist", 2, metadataFileCount(ops.current()));
}
+ @Test
+ public void testLockExceptionUnknownSuccessCommit() 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();
+
+ TableMetadata metadataV2 = ops.current();
+
+ Assertions.assertThat(ops.current().schema().columns().size()).isEqualTo(2);
+
+ HiveTableOperations spyOps = spy(ops);
+
+ // Simulate a communication error after a successful commit
+ doAnswer(
+ i -> {
+ org.apache.hadoop.hive.metastore.api.Table tbl =
+ i.getArgument(0, org.apache.hadoop.hive.metastore.api.Table.class);
+ String location = i.getArgument(2, String.class);
+ ops.persistTable(tbl, true, location);
+ throw new LockException("Datacenter on fire");
+ })
+ .when(spyOps)
+ .persistTable(any(), anyBoolean(), any());
+
+ Assertions.assertThatThrownBy(() -> spyOps.commit(metadataV2, metadataV1))
+ .hasMessageContaining("Failed to heartbeat for hive lock while")
+ .isInstanceOf(CommitStateUnknownException.class);
+
+ ops.refresh();
+
+ Assertions.assertThat(ops.current())
+ .as("Current metadata should have changed")
Review Comment:
Hmm, for now could we just check locations? Then we can do a full equal implementation in another PR
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] ConeyLiu commented on pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "ConeyLiu (via GitHub)" <gi...@apache.org>.
ConeyLiu commented on PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#issuecomment-1615588030
Thanks @pvary @szehon-ho @RussellSpitzer for the reviewing.
> do we need to backport this and do a 1.3.1 release?
It would be better to do that. Should I submit a PR to the 1.3.x branch? I can't find it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] pvary commented on a diff in pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#discussion_r1246122362
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -352,6 +352,49 @@ public void testNoLockThriftExceptionConcurrentCommit() throws TException, Inter
Assert.assertEquals("New metadata files should not exist", 2, metadataFileCount(ops.current()));
}
+ @Test
+ public void testLockExceptionUnknownSuccessCommit() 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();
+
+ TableMetadata metadataV2 = ops.current();
+
+ Assertions.assertThat(ops.current().schema().columns().size()).isEqualTo(2);
+
+ HiveTableOperations spyOps = spy(ops);
+
+ // Simulate a communication error after a successful commit
+ doAnswer(
+ i -> {
+ org.apache.hadoop.hive.metastore.api.Table tbl =
+ i.getArgument(0, org.apache.hadoop.hive.metastore.api.Table.class);
+ String location = i.getArgument(2, String.class);
+ ops.persistTable(tbl, true, location);
+ throw new LockException("Datacenter on fire");
+ })
+ .when(spyOps)
+ .persistTable(any(), anyBoolean(), any());
+
+ Assertions.assertThatThrownBy(() -> spyOps.commit(metadataV2, metadataV1))
+ .hasMessageContaining("Failed to heartbeat for hive lock while")
+ .isInstanceOf(CommitStateUnknownException.class);
+
+ ops.refresh();
+
+ Assertions.assertThat(ops.current())
+ .as("Current metadata should have changed")
Review Comment:
Nit: "Current metadata should have changed" -> should `not` have changed
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] pvary commented on pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#issuecomment-1612451875
Thanks for the fix @ConeyLiu! Let one small comment for the test
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] pvary merged pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #7931:
URL: https://github.com/apache/iceberg/pull/7931
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] ConeyLiu commented on a diff in pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "ConeyLiu (via GitHub)" <gi...@apache.org>.
ConeyLiu commented on code in PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#discussion_r1248611530
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -352,6 +352,49 @@ public void testNoLockThriftExceptionConcurrentCommit() throws TException, Inter
Assert.assertEquals("New metadata files should not exist", 2, metadataFileCount(ops.current()));
}
+ @Test
+ public void testLockExceptionUnknownSuccessCommit() 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();
+
+ TableMetadata metadataV2 = ops.current();
+
+ Assertions.assertThat(ops.current().schema().columns().size()).isEqualTo(2);
+
+ HiveTableOperations spyOps = spy(ops);
+
+ // Simulate a communication error after a successful commit
+ doAnswer(
+ i -> {
+ org.apache.hadoop.hive.metastore.api.Table tbl =
+ i.getArgument(0, org.apache.hadoop.hive.metastore.api.Table.class);
+ String location = i.getArgument(2, String.class);
+ ops.persistTable(tbl, true, location);
+ throw new LockException("Datacenter on fire");
+ })
+ .when(spyOps)
+ .persistTable(any(), anyBoolean(), any());
+
+ Assertions.assertThatThrownBy(() -> spyOps.commit(metadataV2, metadataV1))
+ .hasMessageContaining("Failed to heartbeat for hive lock while")
+ .isInstanceOf(CommitStateUnknownException.class);
+
+ ops.refresh();
+
+ Assertions.assertThat(ops.current())
+ .as("Current metadata should have changed")
Review Comment:
Thanks, @RussellSpitzer for the good suggestion. And I just verified that the `TableMetadata` has not customized the `equals`. So there are some other UTs should be improved. Should we do that in my patch or combine it in this one?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] pvary commented on pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#issuecomment-1617397729
Thanks @ConeyLiu for the PR and @szehon-ho and @RussellSpitzer for the review!
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] pvary commented on pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#issuecomment-1617400363
@ConeyLiu: The 1.3.x branch is missing. Maybe we forgot create it during the release process.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] ConeyLiu commented on pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "ConeyLiu (via GitHub)" <gi...@apache.org>.
ConeyLiu commented on PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#issuecomment-1617649952
Thanks @pvary for merging this and thanks @szehon-ho @RussellSpitzer for the review.
> The 1.3.x branch is missing. Maybe we forgot create it during the release process.
Yeah, there is no way to backport it.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] RussellSpitzer commented on a diff in pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#discussion_r1248311556
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -352,6 +352,49 @@ public void testNoLockThriftExceptionConcurrentCommit() throws TException, Inter
Assert.assertEquals("New metadata files should not exist", 2, metadataFileCount(ops.current()));
}
+ @Test
+ public void testLockExceptionUnknownSuccessCommit() 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();
+
+ TableMetadata metadataV2 = ops.current();
+
+ Assertions.assertThat(ops.current().schema().columns().size()).isEqualTo(2);
+
+ HiveTableOperations spyOps = spy(ops);
+
+ // Simulate a communication error after a successful commit
+ doAnswer(
+ i -> {
+ org.apache.hadoop.hive.metastore.api.Table tbl =
+ i.getArgument(0, org.apache.hadoop.hive.metastore.api.Table.class);
+ String location = i.getArgument(2, String.class);
+ ops.persistTable(tbl, true, location);
+ throw new LockException("Datacenter on fire");
+ })
+ .when(spyOps)
+ .persistTable(any(), anyBoolean(), any());
+
+ Assertions.assertThatThrownBy(() -> spyOps.commit(metadataV2, metadataV1))
+ .hasMessageContaining("Failed to heartbeat for hive lock while")
+ .isInstanceOf(CommitStateUnknownException.class);
+
+ ops.refresh();
+
+ Assertions.assertThat(ops.current())
+ .as("Current metadata should have changed")
Review Comment:
To make this clearer can we instead check that the metadata is the correct new snapshot? We should be able to get this in our mock.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] ConeyLiu commented on a diff in pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "ConeyLiu (via GitHub)" <gi...@apache.org>.
ConeyLiu commented on code in PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#discussion_r1248603627
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -352,6 +352,49 @@ public void testNoLockThriftExceptionConcurrentCommit() throws TException, Inter
Assert.assertEquals("New metadata files should not exist", 2, metadataFileCount(ops.current()));
}
+ @Test
+ public void testLockExceptionUnknownSuccessCommit() 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();
+
+ TableMetadata metadataV2 = ops.current();
+
+ Assertions.assertThat(ops.current().schema().columns().size()).isEqualTo(2);
+
+ HiveTableOperations spyOps = spy(ops);
+
+ // Simulate a communication error after a successful commit
+ doAnswer(
+ i -> {
+ org.apache.hadoop.hive.metastore.api.Table tbl =
+ i.getArgument(0, org.apache.hadoop.hive.metastore.api.Table.class);
+ String location = i.getArgument(2, String.class);
+ ops.persistTable(tbl, true, location);
+ throw new LockException("Datacenter on fire");
+ })
+ .when(spyOps)
+ .persistTable(any(), anyBoolean(), any());
+
+ Assertions.assertThatThrownBy(() -> spyOps.commit(metadataV2, metadataV1))
+ .hasMessageContaining("Failed to heartbeat for hive lock while")
+ .isInstanceOf(CommitStateUnknownException.class);
+
+ ops.refresh();
+
+ Assertions.assertThat(ops.current())
+ .as("Current metadata should have changed")
Review Comment:
@RussellSpitzer, updated to check whether the metadata location and the schema are correct.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] szehon-ho commented on pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#issuecomment-1624115840
@aokolnychyi do you have any suggestion ?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] aokolnychyi commented on pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#issuecomment-1624386986
@szehon-ho, could you raise a discussion thread on the dev list for 1.3.1?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] aokolnychyi commented on pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#issuecomment-1624385774
I can create a branch from the tag.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] ConeyLiu commented on pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "ConeyLiu (via GitHub)" <gi...@apache.org>.
ConeyLiu commented on PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#issuecomment-1611084653
cc @pvary and @szehon-ho, please help to review this when you are free.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] ConeyLiu commented on a diff in pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "ConeyLiu (via GitHub)" <gi...@apache.org>.
ConeyLiu commented on code in PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#discussion_r1248611530
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -352,6 +352,49 @@ public void testNoLockThriftExceptionConcurrentCommit() throws TException, Inter
Assert.assertEquals("New metadata files should not exist", 2, metadataFileCount(ops.current()));
}
+ @Test
+ public void testLockExceptionUnknownSuccessCommit() 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();
+
+ TableMetadata metadataV2 = ops.current();
+
+ Assertions.assertThat(ops.current().schema().columns().size()).isEqualTo(2);
+
+ HiveTableOperations spyOps = spy(ops);
+
+ // Simulate a communication error after a successful commit
+ doAnswer(
+ i -> {
+ org.apache.hadoop.hive.metastore.api.Table tbl =
+ i.getArgument(0, org.apache.hadoop.hive.metastore.api.Table.class);
+ String location = i.getArgument(2, String.class);
+ ops.persistTable(tbl, true, location);
+ throw new LockException("Datacenter on fire");
+ })
+ .when(spyOps)
+ .persistTable(any(), anyBoolean(), any());
+
+ Assertions.assertThatThrownBy(() -> spyOps.commit(metadataV2, metadataV1))
+ .hasMessageContaining("Failed to heartbeat for hive lock while")
+ .isInstanceOf(CommitStateUnknownException.class);
+
+ ops.refresh();
+
+ Assertions.assertThat(ops.current())
+ .as("Current metadata should have changed")
Review Comment:
Thanks, @RussellSpitzer for the good suggestion. And I just verified that the `TableMetadata` has not customized the `equals`. So there are some other UTs should be improved. Should we do that in another patch or combine it in this one?
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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
[GitHub] [iceberg] ConeyLiu commented on a diff in pull request #7931: Hive: Set commit state as Unknown before throwing CommitStateUnknownException
Posted by "ConeyLiu (via GitHub)" <gi...@apache.org>.
ConeyLiu commented on code in PR #7931:
URL: https://github.com/apache/iceberg/pull/7931#discussion_r1249147254
##########
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCommits.java:
##########
@@ -352,6 +352,49 @@ public void testNoLockThriftExceptionConcurrentCommit() throws TException, Inter
Assert.assertEquals("New metadata files should not exist", 2, metadataFileCount(ops.current()));
}
+ @Test
+ public void testLockExceptionUnknownSuccessCommit() 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();
+
+ TableMetadata metadataV2 = ops.current();
+
+ Assertions.assertThat(ops.current().schema().columns().size()).isEqualTo(2);
+
+ HiveTableOperations spyOps = spy(ops);
+
+ // Simulate a communication error after a successful commit
+ doAnswer(
+ i -> {
+ org.apache.hadoop.hive.metastore.api.Table tbl =
+ i.getArgument(0, org.apache.hadoop.hive.metastore.api.Table.class);
+ String location = i.getArgument(2, String.class);
+ ops.persistTable(tbl, true, location);
+ throw new LockException("Datacenter on fire");
+ })
+ .when(spyOps)
+ .persistTable(any(), anyBoolean(), any());
+
+ Assertions.assertThatThrownBy(() -> spyOps.commit(metadataV2, metadataV1))
+ .hasMessageContaining("Failed to heartbeat for hive lock while")
+ .isInstanceOf(CommitStateUnknownException.class);
+
+ ops.refresh();
+
+ Assertions.assertThat(ops.current())
+ .as("Current metadata should have changed")
Review Comment:
Sure, updated.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
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