Conversation
Chaho12
force-pushed
the
feature/jyoo/fix-access-control-exception
branch
from
299a369 to
8764b94
Compare
| addSuppressed(e, suppressedExceptions); | ||
| throw e; | ||
| } | ||
| for (Class<? extends Exception> clazz : stopOnExceptions) { |
There was a problem hiding this comment.
stopOnExceptions passed on here was
0 = {Class@37632} "class io.trino.spi.TrinoException"
1 = {Class@42658} "class io.trino.hive.thrift.metastore.NoSuchObjectException"
2 = {Class@42226} "class java.lang.NullPointerException"
3 = {Class@41494} "class java.lang.IllegalStateException"
4 = {Class@42222} "class java.lang.IllegalArgumentException"
|
Ps. Hive recently sent a PR (Hive 4.3) to handles similar issue in "Propagate HiveAccessControlException to HiveCatalog" It seems that Hive 4.3 will return in ForbiddenException type, so maybe we can add that when checking isAccessControlException |
Chaho12
changed the title
|
Can you take a look at this? @raunaqmorarka |
| { | ||
| // Check the exception message and cause chain for AccessControlException | ||
| // e.g. io.trino.hive.thrift.metastore.MetaException: | ||
| // org.apache.hadoop.security.AccessControlException: Permission denied: ... |
There was a problem hiding this comment.
this could be simplified to:
Throwables.getCausalChain(exception)
.stream()
.map(Throwable::toString)
.anyMatch(message -> message != null && message.contains("AccessControlException"));
8764b94 to
603d8f5
Compare
|
cc @Praveen2112 |
|
It would be great if this PR is included in next release. |
| return Throwables.getCausalChain(exception) | ||
| .stream() | ||
| .map(Throwable::toString) | ||
| .anyMatch(message -> message != null && message.contains("AccessControlException")); |
There was a problem hiding this comment.
Just an idea : maybe we could use the class to check if it is a AccessControlException, also since the class is marked deprecated, so could use the SecurityException instead?
There was a problem hiding this comment.
You mean do this instead of contains?
err.getClass().getName().equals("org.apache.hadoop.fs.permission.AccessControlException")) {
Seems like it is superseded by "org.apache.hadoop.security.AccessControlException"
Nonetheless, depending on Hive version, any of the class name could occur, so we could check for both class
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
|
Any furthere comments? I can rebase it later |
3 participants
Description
Background
This issue occurs when using StorageBasedAuthorizationProvider in Hive Metastore.
In this case, an AccessControlException can be thrown during authorization checks, but the error is not propagated to the client immediately, causing the client to wait until the metastore Thrift timeout expires (default: 10s).
Relevant code:
Fix
This change propagates AccessControlExceptions to the client immediately, avoiding unnecessary waits for the Thrift timeout.
Additional context and related issues
Here is the exception message sent from metastore.
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: