Skip to content

Conversation

ST-DDT
Copy link
Collaborator

@ST-DDT ST-DDT commented Nov 25, 2023

@fengli79
Copy link
Collaborator

Caveat: Better to offload the credential fetching when it's non-cached one. Assuming most of the request can reuse a cached the credential, this may pay a small cost for most of the rpcs.

@ST-DDT
Copy link
Collaborator Author

ST-DDT commented Nov 28, 2023

So would it be better to revert the change and just add a hint to the javadocs?

Or maybe introduce a new variant that it is explicitly called "async" that takes a (executor) -> CompletableFuture<String>?
That way users can return CompletableFuture.completedFuture() if nothing async needs to be done.

private final Function<Executor, CompletableFuture<String>> extraHeadersFutureSupplier;

@Override
        public void applyRequestMetadata(final RequestInfo requestInfo, final Executor appExecutor,
                final MetadataApplier applier) {

            extraHeadersFutureSupplier.apply(executor).whenComplete((header, e) -> {
                if (e == null) {
                    final Metadata extraHeaders = new Metadata();
                    extraHeaders.put(AUTHORIZATION_HEADER, header);
                    applier.apply( extraHeaders);
                } else {
                    applier.fail(Status.UNAUTHENTICATED.withCause(e));
                }
            });
        }

On the other hand we could just let the user implement that temselves.

@fengli79
Copy link
Collaborator

Yes, I feel the dynamic credential doesn't provide too much value to the end users. This is an important enough detail (cached/non-cached/async/blocking) which should be exposed to the implementer of the credential. Either way works, but I personally like the ideal to just let the user implement that themselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature request or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maybe resolve a supplier in the DynamicSecurityHeaderCallCredentials in an executor to avoid blocking the thread
2 participants