-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Enable Happy Eyeballs by default #11022
Conversation
try { | ||
synchronizationContext = helper.getSynchronizationContext(); | ||
} catch (NullPointerException e) { | ||
// If we have a test helper or old helper without a context, we can't schedule a task |
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.
Can we fix the test instead of the code?
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 did fix the tests to provide a synchronization context, but customers could be relying on an old helper that they don't expect to suddenly start failing, so we need to handle the case.
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.
The test I can believe it can be not implemented. But don't mention "test helper" in code comment.
The LB helper should always have getSynchronizationContext()
, from the customer. Originally it is from ManagedChannelImpl. Where is the old helper that does not have this?
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've updated the comment. Despite not having any known examples that are not providing the sync context I don't want to break something unknown out in the wild.
No description provided.