Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: run dynamic call credentials via executor #995

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@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
2 participants