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/10 13:33:34 UTC
[GitHub] [iceberg] aokolnychyi opened a new issue #1904: Make sure unlock is always called in HiveTableOperations
aokolnychyi opened a new issue #1904:
URL: https://github.com/apache/iceberg/issues/1904
We have the following logic in `HiveTableOperations`:
```
} finally {
if (threw) {
// if anything went wrong, clean up the uncommitted metadata file
io().deleteFile(newMetadataLocation);
}
unlock(lockId);
}
```
If `deleteFile` throws an exception, unlock will not be called. I think that is dangerous and should be fixed.
----------------------------------------------------------------
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 issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-742524888
@rdblue @RussellSpitzer, thoughts? I think we can leverage it other places too.
----------------------------------------------------------------
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 issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-742917677
@aokolnychyi, @RussellSpitzer, I opened a PR with an implementation that I think works well: https://github.com/apache/iceberg/pull/1909/files#diff-05514568d4022f3fbd510618bdf1ff14b32271158d385e07879a3181d0b9baaaR40-R46
Lambdas really help here because the classes get ugly. It supports up to 3 checked exceptions. The classes must be passed as arguments, but otherwise it looks roughly the same. There are still some cases that get wrapped in RuntimeException, like any throwable from the main block that isn't a `RuntimeException` or one of the passed checked exceptions. Same with any throwable from the finally block if the main block didn't throw an exception.
----------------------------------------------------------------
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 issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-752048650
I think this one was resolved in PR #1998. Thanks, @jackye1995!
----------------------------------------------------------------
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] pvary commented on issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
pvary commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-743721762
> hi~ @rdblue Shall we consider this situation that process shut down when commit action hold the lock of hive table .
How often could this happen? Asking because Hive have cleaner methods in place which will remove the stale locks which did not receive heartbeat for a given period. So we should clean up the locks as much as possible, but we do not have to consider super rare situations.
----------------------------------------------------------------
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 issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-742524425
Internally, we have a utility method like this:
```
/**
* Execute a block of code and call the failure callbacks in the catch block. If exceptions occur
* in either the catch or the finally block, they are appended to the list of suppressed
* exceptions in original exception which is then rethrown.
*
* This is primarily an issue with `catch { abort() }` or `finally { out.close() }` blocks,
* where the abort/close needs to be called to clean up `out`, but if an exception happened
* in `out.write`, it's likely `out` may be corrupted and `abort` or `out.close` will
* fail as well. This would then suppress the original/likely more meaningful
* exception from the original `out.write` call.
*/
def tryWithSafeFinallyAndFailureCallbacks[T](block: => T)(catchBlock: => Unit = (), finallyBlock: => Unit = ()): T = {
var originalThrowable: Throwable = null
try {
block
} catch {
case cause: Throwable =>
originalThrowable = cause
try {
catchBlock
} catch {
case t: Throwable =>
if (originalThrowable != t) {
originalThrowable.addSuppressed(t)
}
}
throw originalThrowable
} finally {
try {
finallyBlock
} catch {
case t: Throwable if originalThrowable != null && originalThrowable != t =>
originalThrowable.addSuppressed(t)
throw originalThrowable
}
}
}
```
I think we need a similar variant in Java in Iceberg.
----------------------------------------------------------------
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] pvary commented on issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
pvary commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-755118288
> > > hi~ @rdblue Shall we consider this situation that process shut down when commit action hold the lock of hive table .
> >
> >
> > How often could this happen? Asking because Hive have cleaner methods in place which will remove the stale locks which did not receive heartbeat for a given period. So we should clean up the locks as much as possible, but we do not have to consider super rare situations.
>
> hi~@pvary i did not encounter this situation，i just have a guess . could the period of no receive heartbeat be config at hive metastore
The config is called [hive.txn.timeout](https://cwiki.apache.org/confluence/plugins/servlet/mobile?contentId=40509723#ConfigurationProperties-Transactions). The default is 300s. Hope this help.
----------------------------------------------------------------
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 closed issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
aokolnychyi closed issue #1904:
URL: https://github.com/apache/iceberg/issues/1904
----------------------------------------------------------------
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] jacques-n commented on issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
jacques-n commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-742926538
For the original comment from @aokolnychyi , I've generally tried to address this using try-with-resources, which does a much better job of handling multiple exception cases natively then trying to build something functional. For example, we created [AutoCloseableLock](https://github.com/dremio/dremio-oss/blob/master/common/src/main/java/com/dremio/common/concurrent/AutoCloseableLock.java) to manage this particular locking situation.
I also see it as somewhat of an anti-pattern in these complex scenarios to use finally blocks at all as it is too easy for people to make mistakes. The general problem being having multiple clean up tasks of any type. Instead, we typically would use multiple separate try-with-resources resource declarations or if you have more complex scenarios (returning a closeable only if several other operations succeed, each with their own rollback behavior, we created [RollbackCloseable](https://github.com/dremio/dremio-oss/blob/master/common/src/main/java/com/dremio/common/AutoCloseables.java#L176).
----------------------------------------------------------------
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] pvary edited a comment on issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
pvary edited a comment on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-755118288
> > > hi~ @rdblue Shall we consider this situation that process shut down when commit action hold the lock of hive table .
> >
> >
> > How often could this happen? Asking because Hive have cleaner methods in place which will remove the stale locks which did not receive heartbeat for a given period. So we should clean up the locks as much as possible, but we do not have to consider super rare situations.
>
> hi~@pvary i did not encounter this situation，i just have a guess . could the period of no receive heartbeat be config at hive metastore
The config is called [hive.txn.timeout](https://cwiki.apache.org/confluence/display/Hive/Configuration+Properties#ConfigurationProperties-Transactions). The default is 300s. Hope this help.
----------------------------------------------------------------
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] wg1026688210 commented on issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
wg1026688210 commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-755041359
> > hi~ @rdblue Shall we consider this situation that process shut down when commit action hold the lock of hive table .
>
> How often could this happen? Asking because Hive have cleaner methods in place which will remove the stale locks which did not receive heartbeat for a given period. So we should clean up the locks as much as possible, but we do not have to consider super rare situations.
hi~@pvary i did not encounter this situation，i just have a guess . could the period of no receive heartbeat be config at hive metastore
----------------------------------------------------------------
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] wg1026688210 edited a comment on issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
wg1026688210 edited a comment on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-743714128
hi~ @rdblue Shall we consider this situation that process shut down when commit action hold the lock of hive table .
----------------------------------------------------------------
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 issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-743432878
@jacques-n, I think this is a bit simpler case. The problem here is that cleanup is being done in `finally` to avoid modifying the throwables that might be caught, and to ensure that the cleanup is always done, even if the `catch` cases change over time. Catching `Exception` means that it is annoyingly difficult thanks to checked exceptions to throw the exception that was caught.
The PR I opened solves that problem directly. I agree with the idea of using `AutoCloseable` and multiple try-with-resources, but this problem doesn't need multiple blocks. It looks like `RollbackCloseable` does something very similar to the pattern with the `threw` boolean, except that you'd normally use a separate try, which with the `runSafely` method isn't really needed.
----------------------------------------------------------------
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] wg1026688210 commented on issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
wg1026688210 commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-743714128
hi~ @rdblue Shall we consider this situation that Process shut down when commit action hold the lock of hive table .
----------------------------------------------------------------
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 issue #1904: Make sure unlock is always called in HiveTableOperations
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #1904:
URL: https://github.com/apache/iceberg/issues/1904#issuecomment-742894438
This doesn't interact well with checked exceptions. I think that's the primary reason for using the `threw` boolean in Java. If you have a checked exception but don't know what it is, then you can't really declare that a method may throw it. We can get around that with tricks like parameterizing `throws`, but that gets a bit ugly when you have multiple exceptions. Let me see if I can recreate this in a reasonable way for Java.
----------------------------------------------------------------
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