-
Notifications
You must be signed in to change notification settings - Fork 104
chore: migrate to use common EnvironmentProvider interface #1606
Conversation
| private final Executor executor; | ||
| private final HeaderProvider headerProvider; | ||
| private final String endpoint; | ||
| // TODO: remove. envProvider currently provides DirectPath environment variable, and is only used |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
vam-google
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
|
@arithmetic1728 can you please take a look? |
No description provided.