Skip to content
This repository was archived by the owner on Sep 26, 2023. It is now read-only.

Conversation

@chanseokoh
Copy link
Contributor

No description provided.

@chanseokoh chanseokoh requested review from a team, blakeli0 and meltsufin January 21, 2022 16:41
@chanseokoh chanseokoh requested review from a team as code owners January 21, 2022 16:41
private final Executor executor;
private final HeaderProvider headerProvider;
private final String endpoint;
// TODO: remove. envProvider currently provides DirectPath environment variable, and is only used
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where/What is this DirectPath environment variable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea. I just copied the Javadoc comment from line 760.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean the refactoring makes sense, since DirectPathEnvironmentProvider does nothing.
@vam-google Do you know?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont' know details, but @arithmetic1728 should have all the answers.

Copy link
Contributor Author

@chanseokoh chanseokoh Jan 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arithmetic1728 @mohanli-ml said that this may be removed once DirectPath becomes stable, but for now, there's no immediate plan to remove it. The env vars are currently used here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what Direct Path does. The comment wasn't added by me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, I confused you with @mohanli-ml. He has all the context.

Copy link
Contributor

@vam-google vam-google left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but let @arithmetic1728 to take a look first.

private final Executor executor;
private final HeaderProvider headerProvider;
private final String endpoint;
// TODO: remove. envProvider currently provides DirectPath environment variable, and is only used
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont' know details, but @arithmetic1728 should have all the answers.

@vam-google
Copy link
Contributor

@arithmetic1728 can you please take a look?
\

@chanseokoh chanseokoh added the automerge Merge the pull request once unit tests and other checks pass. label Jan 21, 2022
@chanseokoh chanseokoh merged commit de285de into main Jan 21, 2022
@chanseokoh chanseokoh deleted the refactor-code branch January 21, 2022 19:57
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Jan 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants