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/12/28 17:39:05 UTC
[GitHub] [iceberg] jackye1995 opened a new pull request #1998: Hive: ensure unlock is called when new metadata cannot be deleted
jackye1995 opened a new pull request #1998:
URL: https://github.com/apache/iceberg/pull/1998
wrap `io().deleteFile(metadataLocation)` in try catch block so that `unlock(lockId)` must be called in the finally block.
----------------------------------------------------------------
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1998: Hive: ensure unlock is called when new metadata cannot be deleted
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1998:
URL: https://github.com/apache/iceberg/pull/1998#discussion_r549671840
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -367,6 +363,20 @@ private long acquireLock() throws UnknownHostException, TException, InterruptedE
return lockId;
}
+ private void cleanupMetadataAndUnlock(boolean errorThrown, String metadataLocation, Optional<Long> lockId) {
+ try {
+ if (errorThrown) {
+ // if anything went wrong, clean up the uncommitted metadata file
+ io().deleteFile(metadataLocation);
+ }
+ } catch (RuntimeException e) {
+ LOG.error("Fail to cleanup metadata file at {}", metadataLocation, e);
+ throw e;
Review comment:
I think we may leverage `runSafely` that was added recently.
```
ExceptionUtil.runSafely(
() -> {
if (errorThrown) {
// if anything went wrong, clean up the uncommitted metadata file
io().deleteFile(metadataLocation);
}
return Void.TYPE;
},
e -> LOG.error("Failed to cleanup metadata file at {}", metadataLocation, e),
() -> unlock(lockId));
```
----------------------------------------------------------------
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
[GitHub] [iceberg] rdblue commented on a change in pull request #1998: Hive: ensure unlock is called when new metadata cannot be deleted
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1998:
URL: https://github.com/apache/iceberg/pull/1998#discussion_r549448927
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -367,6 +363,20 @@ private long acquireLock() throws UnknownHostException, TException, InterruptedE
return lockId;
}
+ private void cleanupMetadataAndUnlock(boolean errorThrown, String metadataLocation, Optional<Long> lockId) {
+ try {
+ if (errorThrown) {
+ // if anything went wrong, clean up the uncommitted metadata file
+ io().deleteFile(metadataLocation);
+ }
+ } catch (RuntimeException e) {
+ LOG.error("Fail to cleanup metadata file at {}", metadataLocation, e);
+ throw e;
Review comment:
I think this is fine because we don't want to wrap or modify the original exception, and it is helpful to log the metadata location that was not correctly deleted.
----------------------------------------------------------------
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
[GitHub] [iceberg] rdblue merged pull request #1998: Hive: ensure unlock is called when new metadata cannot be deleted
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1998:
URL: https://github.com/apache/iceberg/pull/1998
----------------------------------------------------------------
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
[GitHub] [iceberg] giovannifumarola commented on a change in pull request #1998: Hive: ensure unlock is called when new metadata cannot be deleted
Posted by GitBox <gi...@apache.org>.
giovannifumarola commented on a change in pull request #1998:
URL: https://github.com/apache/iceberg/pull/1998#discussion_r549445722
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -367,6 +363,20 @@ private long acquireLock() throws UnknownHostException, TException, InterruptedE
return lockId;
}
+ private void cleanupMetadataAndUnlock(boolean errorThrown, String metadataLocation, Optional<Long> lockId) {
+ try {
+ if (errorThrown) {
+ // if anything went wrong, clean up the uncommitted metadata file
+ io().deleteFile(metadataLocation);
+ }
+ } catch (RuntimeException e) {
+ LOG.error("Fail to cleanup metadata file at {}", metadataLocation, e);
+ throw e;
Review comment:
Not a fan of Throw within a Catch.
Maybe you can wrap it in something else.
----------------------------------------------------------------
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
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1998: Hive: ensure unlock is called when new metadata cannot be deleted
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1998:
URL: https://github.com/apache/iceberg/pull/1998#discussion_r549671909
##########
File path: hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##########
@@ -367,6 +363,20 @@ private long acquireLock() throws UnknownHostException, TException, InterruptedE
return lockId;
}
+ private void cleanupMetadataAndUnlock(boolean errorThrown, String metadataLocation, Optional<Long> lockId) {
+ try {
+ if (errorThrown) {
+ // if anything went wrong, clean up the uncommitted metadata file
+ io().deleteFile(metadataLocation);
+ }
+ } catch (RuntimeException e) {
+ LOG.error("Fail to cleanup metadata file at {}", metadataLocation, e);
+ throw e;
Review comment:
cc @rdblue @jackye1995 @giovannifumarola
----------------------------------------------------------------
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