-
Notifications
You must be signed in to change notification settings - Fork 151
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
[#779] feat: Grpc server support random port #820
Conversation
Codecov Report
@@ Coverage Diff @@
## master #820 +/- ##
============================================
+ Coverage 57.63% 58.97% +1.33%
- Complexity 2058 2059 +1
============================================
Files 306 292 -14
Lines 14871 12909 -1962
Branches 1221 1224 +3
============================================
- Hits 8571 7613 -958
+ Misses 5808 4864 -944
+ Partials 492 432 -60
... and 19 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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, thanks @xumanbu
this.port = conf.getInteger(RssBaseConf.RPC_SERVER_PORT); | ||
long maxInboundMessageSize = conf.getLong(RssBaseConf.RPC_MESSAGE_MAX_SIZE); | ||
this.rssConf = conf; | ||
this.port = rssConf.getInteger(RssBaseConf.RPC_SERVER_PORT); |
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.
If using the random port, why reserve this var of port
? the listen port
and port
are unclear.
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.
agree with u.in same test case have reuse RssBaseConf(i think it is a mistake), retain
this the port var just reserve the configuration value to avoid test failed.
ShuffleWithRssClientTest
Sorry for the late review. |
What changes were proposed in this pull request?
Why are the changes needed?
Fixs #779
Does this PR introduce any user-facing change?
Yes.
rss.rpc.server.port
could be set as zero.How was this patch tested?
UT