-
Notifications
You must be signed in to change notification settings - Fork 4k
core: Optionally parse targets as RFC 3986 URIs #12573
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
base: master
Are you sure you want to change the base?
Conversation
ejona86
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.
Overall, looks good. There's a little bit of discussion to be had about testing getFlag(). Also, I'll note that the commits titles are missing the modified module, even with none of them touching more than one module.
| * You must save the returned {@link AutoCloseable} and close it in an @After method. | ||
| */ | ||
| public static AutoCloseable setFlagForScope(String flag, boolean value) { | ||
| String oldValue = System.setProperty(flag, Boolean.toString(value)); |
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 generally discourage this approach. It encourages checking the flag every time it is needed, and often the code will crash/misbehave if the value changes over time. You also need to be careful not to call it in hot paths.
Instead, I encourage code using getFlag() to assign it to a static field, and then the test modify that field (optionally through accessors). When possible, the value can also be injected through a constructor or similar instead of modifying the static field.
Brings grpc-java in compliance with the grpc name syntax standard fixing #12244.
This is technically a breaking change because there's a set of target strings that previously parsed fine under java.net.URI but will now throw. There are 3 known cases:
Even though all these are unlikely, we guard the behavior change behind the
GRPC_ENABLE_RFC3986_URISflag (off by default). Technically NameResolverProvider is a public API so there could be resolvers out in the world that we haven't considered.Each commit is meant to be reviewed individually -- I do not plan to squash.