Add retry and jmx metrics while generating Thrift's delegation token #21000

osscm · 2024-03-09T09:15:19Z

Description

Additional context and related issues

fixes : #20999

Release notes

(x) This is not user-visible or is docs only, and no release notes are required.

osscm · 2024-03-09T17:28:52Z

cc @electrum if you can please help with the review, thanks a lot!

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

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

osscm · 2024-04-09T08:32:50Z

@electrum will cross check the pt failures.

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

electrum · 2024-04-10T18:22:26Z

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

+                .withMaxDuration(java.time.Duration.of(30, SECONDS))
+                .withMaxAttempts(maxRetries + 1)
+                .withBackoff(minBackoffDelay.toMillis(), maxBackoffDelay.toMillis(), MILLIS, backoffScaleFactor)
+                .abortOn(throwable -> Throwables.getCausalChain(throwable).stream().anyMatch(TrinoException.class::isInstance))


I'm not sure what errors the metastore throws for getDelegationToken(). The only declared exception is MetaException. If that is always a permanent error, then we should abort on that. Otherwise, we should retry everything. So this should be

.abortOn(MetaException.class)

or remove it if MetaException is thrown for transient errors

wondering, if we can abort on TException?

as getDT throws MetaException, TException,
and MetaException extends TException that extends Exception

public String getDelegationToken(String token_owner, String renewer_kerberos_principal_name) throws MetaException, TException { this.sendGetDelegationToken(token_owner, renewer_kerberos_principal_name); return this.recvGetDelegationToken(); }

so, .abortOn(TException.class)

like it is done above, make sense?

@electrum thanks for the review and help!

Apart from this, I have tried to take care of all the other comments, if you can please check once.

Please see if .abortOn(TException.class) make sense or not as TException is also thrown.
Or may be I am missing something?

thanks

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

github-actions · 2024-05-10T17:40:22Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-05-13T16:16:29Z

@osscm .. can you address the final remarks from @electrum .. I think this is ready to merge afterwards since he approved. Any concerns @findinpath ?

osscm · 2024-05-15T23:47:54Z

Ack @Manfred

…

On Mon, May 13, 2024 at 9:16 AM Manfred Moser ***@***.***> wrote: @osscm <https://github.com/osscm> .. can you address the final remarks from @electrum <https://github.com/electrum> .. I think this is ready to merge afterwards since he approved. Any concerns @findinpath <https://github.com/findinpath> ? — Reply to this email directly, view it on GitHub <#21000 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AXQ2PYWXQAF3TZKZTRUITK3ZCDRPLAVCNFSM6AAAAABEN6XRJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBYGEZDSNBSGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

github-actions · 2024-06-18T17:02:57Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

github-actions · 2024-07-10T17:02:48Z

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

mosabua · 2024-07-10T18:41:16Z

@electrum can you look .. seems like you approved already. Could you merge? I will see what the CI build does ...

sopel39 · 2024-08-01T12:32:29Z

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

@@ -58,6 +66,12 @@ public TokenFetchingMetastoreClientFactory(
                        .maximumSize(thriftConfig.getDelegationTokenCacheMaximumSize()),
                CacheLoader.from(this::loadDelegationToken));
        this.refreshPeriod = Duration.ofMinutes(1).toNanos();
+        retryPolicy = RetryPolicy.builder()
+                .withMaxDuration(java.time.Duration.of(30, SECONDS))


use maxRetryTime from ThriftHiveMetastore

sopel39 · 2024-08-01T12:39:22Z

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

+                .withMaxDuration(java.time.Duration.of(30, SECONDS))
+                .withMaxAttempts(thriftConfig.getMaxRetries() + 1)
+                .withBackoff(thriftConfig.getMinBackoffDelay().toMillis(), thriftConfig.getMaxBackoffDelay().toMillis(), MILLIS, thriftConfig.getBackoffScaleFactor())
+                .abortOn(throwable -> Throwables.getCausalChain(throwable).stream().anyMatch(TrinoException.class::isInstance))


Did we figure which exceptions are permanent or temporary?
Is MetaException permanent?
Can TException be temporary?

Would getDelegationToken throw TrinoExceptipon at all (I don't think it does)?

yes, we do not need to use TrinoException,

we can Abort on TException based on how it is done for other operation's retries as well,
like createDatabase at the Metastore side throws three exceptions and Trino side we abort for these three exceptions only.

I though that maybe we can narrow it more, but it's fine for me

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

cla-bot · 2024-09-01T06:43:59Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: mmalhotra.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

sopel39

lgtm % comments

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

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

plugin/trino-hive/src/main/java/io/trino/plugin/hive/util/RetryDriver.java

sopel39 · 2024-09-02T12:11:44Z

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

+                .withMaxDuration(java.time.Duration.of(30, SECONDS))
+                .withMaxAttempts(thriftConfig.getMaxRetries() + 1)
+                .withBackoff(thriftConfig.getMinBackoffDelay().toMillis(), thriftConfig.getMaxBackoffDelay().toMillis(), MILLIS, thriftConfig.getBackoffScaleFactor())
+                .abortOn(throwable -> Throwables.getCausalChain(throwable).stream().anyMatch(TrinoException.class::isInstance))


I though that maybe we can narrow it more, but it's fine for me

sopel39 · 2024-09-02T12:59:48Z

Please squash commits

sopel39 · 2024-09-03T21:11:24Z

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

@@ -121,4 +144,13 @@ private record DelegationToken(long writeTimeNanos, String delegationToken)
            requireNonNull(delegationToken, "delegationToken is null");
        }
    }
+
+    private static RuntimeException propagate(Throwable throwable)


please remove this method as Failsafe does handle InterruptiodException correctly, see: dev.failsafe.internal.RetryPolicyExecutor#apply

sopel39 · 2024-09-03T21:17:40Z

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

        }
-        catch (TException e) {
-            throw new TrinoException(HIVE_METASTORE_ERROR, e);
+        catch (Exception e) {


this should be:

catch (FailsafeException e) { if (e.getCause() instanceof TException) { throw new TrinoException(HIVE_METASTORE_ERROR, e.getCause()); } throw e; }

per dev.failsafe.FailsafeExecutor#get(dev.failsafe.function.CheckedSupplier<T>) javadoc:

* @throws NullPointerException if the {@code supplier} is null * @throws FailsafeException if the execution fails with a checked Exception. {@link FailsafeException#getCause()} can * be used to learn the underlying checked exception.

cla-bot · 2024-09-24T15:32:43Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: mmalhotra.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

sopel39

lgtm % failure is related % please squash commits

osscm · 2024-10-17T15:01:31Z

lgtm % failure is related % please squash commits

updated, can we merge it now, please?

cla-bot bot added the cla-signed label Mar 9, 2024

github-actions bot added tests:hive hive Hive connector labels Mar 9, 2024

osscm changed the title ~~Add retry and jmx means for the Thrift's delegation token generation~~ Add retry and jmx metrics while generating Thrift's delegation token Mar 9, 2024

electrum reviewed Mar 25, 2024

View reviewed changes

...src/main/java/io/trino/plugin/hive/metastore/thrift/TokenFetchingMetastoreClientFactory.java Outdated Show resolved Hide resolved

...src/main/java/io/trino/plugin/hive/metastore/thrift/TokenFetchingMetastoreClientFactory.java Outdated Show resolved Hide resolved

osscm force-pushed the thrift-dt-retry branch 3 times, most recently from ba0ed22 to 7b7d7b8 Compare April 8, 2024 21:43

findinpath reviewed Apr 9, 2024

View reviewed changes

...src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreAuthenticationModule.java Show resolved Hide resolved

osscm force-pushed the thrift-dt-retry branch from 59beb06 to 52c6491 Compare April 10, 2024 09:01

electrum approved these changes Apr 10, 2024

View reviewed changes

electrum reviewed Apr 10, 2024

View reviewed changes

...src/main/java/io/trino/plugin/hive/metastore/thrift/TokenFetchingMetastoreClientFactory.java Outdated Show resolved Hide resolved

electrum reviewed Apr 10, 2024

View reviewed changes

osscm force-pushed the thrift-dt-retry branch from 52c6491 to 78b048a Compare April 10, 2024 19:23

findepi removed the tests:hive label Apr 18, 2024

github-actions bot added the stale label May 10, 2024

github-actions bot removed the stale label May 13, 2024

osscm force-pushed the thrift-dt-retry branch 3 times, most recently from 7024c36 to d21f431 Compare May 28, 2024 06:52

github-actions bot added the stale label Jun 18, 2024

github-actions bot closed this Jul 10, 2024

mosabua reopened this Jul 10, 2024

mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Jul 10, 2024

sopel39 reviewed Aug 1, 2024

View reviewed changes

cla-bot bot removed the cla-signed label Sep 1, 2024

osscm force-pushed the thrift-dt-retry branch from 9cb45ac to e1343f7 Compare September 2, 2024 01:29

cla-bot bot added the cla-signed label Sep 2, 2024

sopel39 approved these changes Sep 2, 2024

View reviewed changes

osscm force-pushed the thrift-dt-retry branch from e1343f7 to 0c20882 Compare September 2, 2024 21:10

sopel39 approved these changes Sep 3, 2024

View reviewed changes

cla-bot bot removed the cla-signed label Sep 24, 2024

osscm force-pushed the thrift-dt-retry branch from 10d490b to 08d8e64 Compare September 27, 2024 15:33

cla-bot bot added the cla-signed label Sep 27, 2024

osscm force-pushed the thrift-dt-retry branch from 08d8e64 to 59c19e0 Compare September 27, 2024 15:37

sopel39 approved these changes Sep 30, 2024

View reviewed changes

osscm force-pushed the thrift-dt-retry branch from 59c19e0 to 3202480 Compare October 17, 2024 14:55

sopel39 force-pushed the thrift-dt-retry branch 2 times, most recently from 31f0928 to a3e599c Compare October 18, 2024 17:41

Add retry and jmx means for the Thrift's delegation token generation

6a691da

sopel39 force-pushed the thrift-dt-retry branch from a3e599c to 6a691da Compare October 18, 2024 19:04

sopel39 added the no-release-notes This pull request does not require release notes entry label Oct 18, 2024

sopel39 merged commit f78c99f into trinodb:master Oct 18, 2024
56 checks passed

github-actions bot added this to the 463 milestone Oct 18, 2024

mosabua mentioned this pull request Oct 18, 2024

Add Trino 463 release notes #23810

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retry and jmx metrics while generating Thrift's delegation token #21000

Add retry and jmx metrics while generating Thrift's delegation token #21000

osscm commented Mar 9, 2024 •

edited by mosabua

Loading

osscm commented Mar 9, 2024

osscm commented Apr 9, 2024

electrum Apr 10, 2024

osscm Apr 10, 2024 •

edited

Loading

osscm May 27, 2024

github-actions bot commented May 10, 2024

mosabua commented May 13, 2024

osscm commented May 15, 2024 via email

github-actions bot commented Jun 18, 2024

github-actions bot commented Jul 10, 2024

mosabua commented Jul 10, 2024 •

edited

Loading

sopel39 Aug 1, 2024

sopel39 Aug 1, 2024

osscm Aug 31, 2024 •

edited

Loading

sopel39 Sep 2, 2024

cla-bot bot commented Sep 1, 2024

sopel39 left a comment

sopel39 Sep 2, 2024

sopel39 commented Sep 2, 2024

sopel39 Sep 3, 2024

sopel39 Sep 3, 2024 •

edited

Loading

cla-bot bot commented Sep 24, 2024

sopel39 left a comment •

edited

Loading

osscm commented Oct 17, 2024 •

edited

Loading

Add retry and jmx metrics while generating Thrift's delegation token #21000

Add retry and jmx metrics while generating Thrift's delegation token #21000

Conversation

osscm commented Mar 9, 2024 • edited by mosabua Loading

Description

Additional context and related issues

Release notes

osscm commented Mar 9, 2024

osscm commented Apr 9, 2024

electrum Apr 10, 2024

Choose a reason for hiding this comment

osscm Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

osscm May 27, 2024

Choose a reason for hiding this comment

github-actions bot commented May 10, 2024

mosabua commented May 13, 2024

osscm commented May 15, 2024 via email

github-actions bot commented Jun 18, 2024

github-actions bot commented Jul 10, 2024

mosabua commented Jul 10, 2024 • edited Loading

sopel39 Aug 1, 2024

Choose a reason for hiding this comment

sopel39 Aug 1, 2024

Choose a reason for hiding this comment

osscm Aug 31, 2024 • edited Loading

Choose a reason for hiding this comment

sopel39 Sep 2, 2024

Choose a reason for hiding this comment

cla-bot bot commented Sep 1, 2024

sopel39 left a comment

Choose a reason for hiding this comment

sopel39 Sep 2, 2024

Choose a reason for hiding this comment

sopel39 commented Sep 2, 2024

sopel39 Sep 3, 2024

Choose a reason for hiding this comment

sopel39 Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

cla-bot bot commented Sep 24, 2024

sopel39 left a comment • edited Loading

Choose a reason for hiding this comment

osscm commented Oct 17, 2024 • edited Loading

osscm commented Mar 9, 2024 •

edited by mosabua

Loading

osscm Apr 10, 2024 •

edited

Loading

mosabua commented Jul 10, 2024 •

edited

Loading

osscm Aug 31, 2024 •

edited

Loading

sopel39 Sep 3, 2024 •

edited

Loading

sopel39 left a comment •

edited

Loading

osscm commented Oct 17, 2024 •

edited

Loading