Allow named channels to inherit default configuration #347
Conversation
o-shevchenko
force-pushed
the
gh-345
branch
2 times, most recently
from
707a333 to
1218da9
Compare
o-shevchenko
force-pushed
the
gh-345
branch
2 times, most recently
from
c8c0055 to
04de180
Compare
o-shevchenko
changed the title
onobc
left a comment
onobc
left a comment
There was a problem hiding this comment.
Thank you for another awesome contribution @o-shevchenko . I have left a few comments, other than that we are close to merge.
...e/src/main/java/org/springframework/boot/grpc/client/autoconfigure/GrpcClientProperties.java
Outdated
Show resolved
Hide resolved
| */ | ||
| public static class ChannelConfig { | ||
|
|
||
| private static final String DEFAULT_ADDRESS = "static://localhost:9090"; |
There was a problem hiding this comment.
I think these are good changes (adding @Nullable and default constants) but let's do them in a separate PR and keep this focused solely on merging / default channels.
There was a problem hiding this comment.
PropertyMapper copies ALL values including defaults. The merging can't distinguish between "explicitly set to 4MB" vs "default value is 4MB".
For merging to work correctly, we need nullable fields.
I will use PropertyMapper but with nullable field change, don't see how we can avoid that for this PR.
I will reduce number of changes and delete defaults as well.
There was a problem hiding this comment.
Ahh yeh. Ok, this makes sense. I will re-review w/ this in mind.
There was a problem hiding this comment.
Thanks for the updates @o-shevchenko . One minor ask, in the future just push additional commits rather than force push as that makes it more difficult for the reviewer to pick up where they left off (changes etc..)..
There was a problem hiding this comment.
ok, will do
I just see that you don't squash commits before merge, so history become hard to read
There was a problem hiding this comment.
I see in Contribution two suggestions:
- Choose the granularity of your commits consciously and squash commits that represent multiple edits or corrections of the same logical change. See Rewriting History section of Pro Git for an overview of streamlining commit history.
- If asked to make corrections, simply push the changes against the same branch, and your pull request will be updated. In other words, you do not need to create a new pull request when asked to make changes.
Therefore, I decided to do force push (which I have to do as well) but it will look better after the merge. My previous merged PR landed with multiple commits that actually can be in one.
https://github.com/spring-projects/spring-grpc/blob/main/CONTRIBUTING.md
I think adjusting (1) or adding squash commits will help.
There was a problem hiding this comment.
ok, will do I just see that you don't squash commits before merge, so history become hard to read
I personally always squash commits and hygiene them locally prior to merging. Its possible that some PR may have been merged w/ multiple commits but I am a firm believer of a linear git history as well 😸. So thank you for thinking of that - much appreciated.
And I totally agree, the CONTRIBUTING.md was likely copied from another repo and is probably out of date. I created #358 to prevent me from forgetting about this 😄
...e/src/main/java/org/springframework/boot/grpc/client/autoconfigure/GrpcClientProperties.java
Show resolved
Hide resolved
Signed-off-by: Oleksandr Shevchenko <oleksandr.shevchenko@datarobot.com>
2 participants
Named channels now inherit settings from default configuration. Explicitly configured values in named channels take precedence over defaults.
[resolves #345]