retry to connect to hive metastore with new delegation token. #10561

Sunwoo-Shin · 2022-01-12T10:12:10Z

cla-bot · 2022-01-12T10:12:12Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

Sunwoo-Shin · 2022-01-12T10:18:22Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

CLA had sent to [email protected]
( Sent Email: [email protected] )

findepi · 2022-01-12T10:31:39Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

+        throws TException
+    {
+        if (refresh) {
+            delegationTokenCache.refresh(username);


refresh might be async (under what conditions?). it should be simpler to invalidate, but this would be vulnerable to #10512 / google/guava#1881.

cc @kokosing

I think we should not use refresh. It makes it hard to reason about the current state. Notice that we don't know if below getUnchecked would return new or old value.

With the link below, it was confirmed that refresh function works as async. Therefore, it has been modified from refresh function to put function.

https://github.com/google/guava/blob/master/guava/src/com/google/common/cache/LocalCache.java#L2364

findepi · 2022-01-12T10:32:09Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

        }

        ThriftMetastoreClient client = createMetastoreClient();
        setMetastoreUserOrClose(client, username);
        return client;
    }

+    private ThriftMetastoreClient createMetastoreClientWithAuthentication(String username, boolean refresh)


Extract:

private String getDelegationToken(String username) { String delegationToken; try { delegationToken = delegationTokenCache.getUnchecked(username); } catch (UncheckedExecutionException e) { throwIfInstanceOf(e.getCause(), TrinoException.class); throw e; } return delegationToken; }

and inline this back:

if (authenticationEnabled) { try { return clientProvider.createMetastoreClient(Optional.of(getDelegationToken(username))); } catch (TException e) { log.debug(e, "It may be a delegation token permission failure, so try again with a new delegation token"); delegationTokenCache.refresh(username); return clientProvider.createMetastoreClient(Optional.of(getDelegationToken(username))); } }

modified in that way. please check again.

cla-bot · 2022-01-12T12:03:56Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

ebyhr · 2022-04-18T21:50:15Z

@cla-bot check

cla-bot · 2022-04-18T21:50:17Z

The cla-bot has been summoned, and re-checked this pull request!

mosabua · 2022-11-03T23:23:18Z

👋 @Sunwoo-Shin - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that. As a first step you would need to rebase.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

Sunwoo-Shin · 2022-11-04T04:39:01Z

@mosabua
Thanks for noticing. I was missing this PR.
now rebase is complete.
It was confirmed that the logic was changed from the ThriftHiveMetastore class to the TokenFetchingMetastoreClientFactory class, and I modified it accordingly.

cc. @findepi

losipiuk · 2022-11-04T10:30:59Z

...src/main/java/io/trino/plugin/hive/metastore/thrift/TokenFetchingMetastoreClientFactory.java

+            return clientProvider.createMetastoreClient(Optional.of(getDelegationToken(username)));
+        }
+        catch (TException e) {
+            log.debug(e, "It may be a delegation token permission failure, so try again with a new delegation token");


I do not think we need this log here. Trino will not be run with debug log level enabled anyway and the log message does not provide any actionable information..

Thank for giving feedbacks :)
I also agree with what you said. this log has been removed.

losipiuk · 2022-11-04T10:31:47Z

...src/main/java/io/trino/plugin/hive/metastore/thrift/TokenFetchingMetastoreClientFactory.java

-            delegationToken = delegationTokenCache.getUnchecked(username);
+            return clientProvider.createMetastoreClient(Optional.of(getDelegationToken(username)));
+        }
+        catch (TException e) {


can we be look deeper into exception to determine if regenerating delegatin token can really be helpful?

An exception is occurring as below.

Trino Error (402 version)

Hive Metastore Error (3.1.2 version)

Maybe sth like that

@Override public ThriftMetastoreClient createMetastoreClientFor(Optional<ConnectorIdentity> identity) throws TException { if (!impersonationEnabled) { return createMetastoreClient(); } String username = identity.map(ConnectorIdentity::getUser) .orElseThrow(() -> new IllegalStateException("End-user name should exist when metastore impersonation is enabled")); try { return clientProvider.createMetastoreClient(Optional.of(getDelegationToken(username))); } catch (TException e) { if (isDelegationTokenExpiration(e)) { // try once again with new token String delegationToken = loadDelegationToken(username); delegationTokenCache.put(username, delegationToken); return clientProvider.createMetastoreClient(Optional.of(delegationToken)); } throw e; } } private boolean isDelegationTokenExpiration(Throwable exception) { if (exception instanceof SecretManager.InvalidToken) { return true; } if (exception.getCause() == null || exception.getCause() == exception) { return false; } return (isDelegationTokenExpiration(exception.getCause())); }

Unfortunately, the message passed to the thrift response cannot be identified as SecretManager.InvalidToken because it is transmitted only with the following string information.

TSaslTransport

Oh - I misread your previous message. The "cause" does not make it through the "TTransportException". :/

Ok - let's go with that. Just please add a comment in catch clause why are you reloading the token.

Also please add a code which ensures that we are not refreshing the token "too often", I think ensuring at most 1 refresh per minute will be fine. We do not want to start hammering metastore which returns errors, because it is overloaded with twice as many requests.

Just please add a comment in catch clause why are you reloading the token.

Added a comment to that.

Also please add a code which ensures that we are not refreshing the token "too often", I think ensuring at most 1 refresh per minute will be fine. We do not want to start hammering metastore which returns errors, because it is overloaded with twice as many requests.

I also positively agree with that.
Since tokens are generated for each user and each expires, the refresh period check is implemented to be checked for each user.

losipiuk · 2022-11-04T10:33:44Z

...src/main/java/io/trino/plugin/hive/metastore/thrift/TokenFetchingMetastoreClientFactory.java

+        catch (TException e) {
+            log.debug(e, "It may be a delegation token permission failure, so try again with a new delegation token");
+            delegationTokenCache.put(username, loadDelegationToken(username));
+            return clientProvider.createMetastoreClient(Optional.of(getDelegationToken(username)));


you can store the delegation token obtained from loadDelegationToken(username)) on variable; put it in the cache and then use it here without going through cache

Thank for giving feedbacks :)
changed it to be reused as a local variable.

losipiuk

Please squash commits and make commit message:

Regenerate Hive delegation token when expired

Sunwoo-Shin · 2022-11-08T17:05:26Z

@losipiuk

Please squash commits and make commit message:
Regenerate Hive delegation token when expired

squash commit and modified it with the written message.

phd3

lgtm

losipiuk · 2022-11-14T09:52:10Z

THanks.
I squashed the commits. Made record definition private, and added null check. WIll merge when CI passes.

Chaho12 · 2022-11-25T01:13:28Z

CI failed :'(

ebyhr · 2022-11-25T01:21:18Z

Both failures are unrelated to this change as far as I confirmed. #14814 & #13288

losipiuk · 2022-12-20T08:21:26Z

THanks.
I squashed the commits. Made record definition private, and added null check. WIll merge when CI passes.

And I totally forgot 🤦 . Sorry

colebow · 2022-12-21T18:51:33Z

@Sunwoo-Shin @losipiuk does this need a release note?

losipiuk · 2022-12-21T19:30:47Z

@Sunwoo-Shin @losipiuk does this need a release note?

@colebow maybe sth like:

Refresh the Hive metastore delegation token to prevent query failures if the metastore was restarted mid-query.

Sunwoo-Shin mentioned this pull request Jan 12, 2022

Suggestions : Hive Metastore Delegation-Token Authentication Failure #10534

Closed

findepi reviewed Jan 12, 2022

View reviewed changes

findepi requested review from kokosing and losipiuk January 12, 2022 10:32

Sunwoo-Shin requested a review from findepi January 13, 2022 01:43

cla-bot bot added the cla-signed label Apr 18, 2022

Sunwoo-Shin force-pushed the feature/retry-connection-with-new-delegation-token branch from f90f01d to 24ad7e0 Compare November 4, 2022 04:23

github-actions bot added the tests:hive label Nov 4, 2022

Sunwoo-Shin force-pushed the feature/retry-connection-with-new-delegation-token branch 2 times, most recently from 7bc1d7f to eeb767c Compare November 4, 2022 04:27

losipiuk reviewed Nov 4, 2022

View reviewed changes

Sunwoo-Shin requested a review from losipiuk November 8, 2022 03:55

losipiuk reviewed Nov 8, 2022

View reviewed changes

Sunwoo-Shin force-pushed the feature/retry-connection-with-new-delegation-token branch from ad8fcfb to 4715b43 Compare November 8, 2022 17:00

Sunwoo-Shin requested a review from losipiuk November 8, 2022 17:17

Sunwoo-Shin force-pushed the feature/retry-connection-with-new-delegation-token branch 2 times, most recently from 010cbf6 to 53fb610 Compare November 14, 2022 01:20

phd3 approved these changes Nov 14, 2022

View reviewed changes

losipiuk force-pushed the feature/retry-connection-with-new-delegation-token branch from 53fb610 to 43473d3 Compare November 14, 2022 09:50

Regenerate Hive delegation token when expired

b99d643

Sunwoo-Shin force-pushed the feature/retry-connection-with-new-delegation-token branch from 43473d3 to b99d643 Compare December 20, 2022 06:33

losipiuk merged commit 3a5e287 into trinodb:master Dec 20, 2022

github-actions bot added this to the 404 milestone Dec 20, 2022

colebow mentioned this pull request Dec 21, 2022

Add Trino 405 release notes #15139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

retry to connect to hive metastore with new delegation token. #10561

retry to connect to hive metastore with new delegation token. #10561

Sunwoo-Shin commented Jan 12, 2022

cla-bot bot commented Jan 12, 2022

Sunwoo-Shin commented Jan 12, 2022

findepi Jan 12, 2022

kokosing Jan 12, 2022

Sunwoo-Shin Jan 12, 2022

findepi Jan 12, 2022

Sunwoo-Shin Jan 12, 2022

cla-bot bot commented Jan 12, 2022

ebyhr commented Apr 18, 2022

cla-bot bot commented Apr 18, 2022

mosabua commented Nov 3, 2022

Sunwoo-Shin commented Nov 4, 2022

losipiuk Nov 4, 2022

Sunwoo-Shin Nov 8, 2022

losipiuk Nov 4, 2022

Sunwoo-Shin Nov 8, 2022

losipiuk Nov 8, 2022

Sunwoo-Shin Nov 8, 2022

losipiuk Nov 10, 2022

losipiuk Nov 10, 2022 •

edited

Loading

Sunwoo-Shin Nov 11, 2022

losipiuk Nov 4, 2022

Sunwoo-Shin Nov 8, 2022

losipiuk left a comment

Sunwoo-Shin commented Nov 8, 2022

phd3 left a comment

losipiuk commented Nov 14, 2022

Chaho12 commented Nov 25, 2022

ebyhr commented Nov 25, 2022 •

edited

Loading

losipiuk commented Dec 20, 2022

colebow commented Dec 21, 2022

losipiuk commented Dec 21, 2022

retry to connect to hive metastore with new delegation token. #10561

retry to connect to hive metastore with new delegation token. #10561

Conversation

Sunwoo-Shin commented Jan 12, 2022

cla-bot bot commented Jan 12, 2022

Sunwoo-Shin commented Jan 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cla-bot bot commented Jan 12, 2022

ebyhr commented Apr 18, 2022

cla-bot bot commented Apr 18, 2022

mosabua commented Nov 3, 2022

Sunwoo-Shin commented Nov 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trino Error (402 version)

Hive Metastore Error (3.1.2 version)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

losipiuk left a comment

Choose a reason for hiding this comment

Sunwoo-Shin commented Nov 8, 2022

phd3 left a comment

Choose a reason for hiding this comment

losipiuk commented Nov 14, 2022

Chaho12 commented Nov 25, 2022

ebyhr commented Nov 25, 2022 • edited Loading

losipiuk commented Dec 20, 2022

colebow commented Dec 21, 2022

losipiuk commented Dec 21, 2022

losipiuk Nov 10, 2022 •

edited

Loading

ebyhr commented Nov 25, 2022 •

edited

Loading