Add retry and jmx metrics while generating Thrift's delegation token #21000
Conversation
osscm
changed the title
|
cc @electrum if you can please help with the review, thanks a lot! |
...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
...src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreAuthenticationModule.java
Show resolved
Hide resolved
|
@electrum will cross check the pt failures. |
...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
| .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)) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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
Show resolved
Hide resolved
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
@osscm .. can you address the final remarks from @electrum .. I think this is ready to merge afterwards since he approved. Any concerns @findinpath ? |
|
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:
***@***.***>
|
osscm
force-pushed
the
thrift-dt-retry
branch
2 times, most recently
from
35f8695
to
7024c36
Compare
|
This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua |
|
Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time. |
|
@electrum can you look .. seems like you approved already. Could you merge? I will see what the CI build does ... |
mosabua
added
stale-ignore
| @@ -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)) | |||
There was a problem hiding this comment.
use maxRetryTime from ThriftHiveMetastore
| .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)) |
There was a problem hiding this comment.
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)?
| } | ||
| catch (TException e) { |
There was a problem hiding this comment.
why not just
catch (TException e) {
throw new TrinoException(HIVE_METASTORE_ERROR, e);
}
?
Description
Additional context and related issues
fixes : #20999
Release notes
( ) 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: