fix: Make it explicit that there is a network call to MDS to get SecureSessionAgentConfig#1573
Conversation
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
| */ | ||
| @ThreadSafe | ||
| public final class S2A { | ||
| public class S2A { |
There was a problem hiding this comment.
On a second look, do you have concerns if we can make this name written out specifically (SecureSessionAgent.java) and (SecureSessionAgentConfig.java) as S2A may not be well known and specific to Google?
| /** @return the cached S2AConfig. */ | ||
| public S2AConfig getConfigFromMDS() { | ||
| return config; | ||
| } |
There was a problem hiding this comment.
What I had in mind was
public SecureSessionAgentConfig getConfig() {
return getS2AConfigFromMDS();
}
as this getConfig() call is the explicit call that will make the network call.
Plus with javadocs that mention that this is a network call?
There was a problem hiding this comment.
I see, so right now the config is being set in the builder and is a member of the class. The current setup ensures that only one call to MDS is made (since there is no need to make the request to MDS multiple times) no matter how many times a user calls getConfigFromMds.
I'm fine with this though, it shifts responsibility to the user to store config and not call getConfig() more than once to save latency of their application. This seems aligned with what you mentioned in our offline discussion yesterday about how it should be explicit a network call is being made.
Done in: 381efe4
| return config; | ||
| } | ||
|
|
||
| public static Builder newBuilder() { |
There was a problem hiding this comment.
Also, if you want, you can include a static create() helper that creates a default instance (i.e. SecureSessionAgent.create()).
product-auto-label
Bot
added
size: m
|
Thanks for review @lqiu96 ! |
lqiu96
left a comment
lqiu96
left a comment
There was a problem hiding this comment.
LGTM. Added a few nits/ questions
|
@rmehta19 Also can you update the title with |
rmehta19
changed the title
rmehta19
changed the title
rmehta19
changed the title
|
@lqiu96 , Thanks for the review! Addressed comments. |
LGTM. Let me know if you feel strongly about final/ non-final. I'll approve again and we can merge then |
|
FYI the linter is failing. I think you need to run |
As discussed offline, let's keep it final so that we can test it from Gax where it is used |
|
2 participants
Make the classes non-final while we're here to allow for mocking in tests if needed.