-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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
[ISSUE #6537] Fix bug MessageStoreConfig.haListenPort item config not… #6538
Conversation
…ig not effective
@@ -167,7 +167,6 @@ public static BrokerController buildBrokerController(String[] args) throws Excep | |||
System.exit(-4); | |||
} | |||
|
|||
messageStoreConfig.setHaListenPort(nettyServerConfig.getListenPort() + 1); |
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 the user has not configured it, could the default port for HA be set to nettyServerConfig.getListenPort() + 1 ?
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.
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.
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.
IMO this PR will break the compatibility, that is, the default value of haLitenPort
is listenPort + 1
. Users setting listenPort
but not setting haLitenPort
explicitly will find haLitenPort
changes after updating to new version.
The more appropriate way may be:
- if
haLitenPort
is set explicitly, use the value set by user; - if
haLitenPort
is not set, assignlistenPort + 1
to it.
You have successfully added a new CodeQL configuration |
@RongtongJin I see that you have approved it, but ci test not pass, what do i need todo |
You can trigger CI again by command: |
Codecov Report
@@ Coverage Diff @@
## develop #6538 +/- ##
=============================================
+ Coverage 43.07% 43.09% +0.01%
- Complexity 8990 8991 +1
=============================================
Files 1107 1107
Lines 78289 78291 +2
Branches 10203 10204 +1
=============================================
+ Hits 33725 33740 +15
+ Misses 40341 40324 -17
- Partials 4223 4227 +4
... and 21 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -167,7 +167,6 @@ public static BrokerController buildBrokerController(String[] args) throws Excep | |||
System.exit(-4); | |||
} | |||
|
|||
messageStoreConfig.setHaListenPort(nettyServerConfig.getListenPort() + 1); |
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.
IMO this PR will break the compatibility, that is, the default value of haLitenPort
is listenPort + 1
. Users setting listenPort
but not setting haLitenPort
explicitly will find haLitenPort
changes after updating to new version.
The more appropriate way may be:
- if
haLitenPort
is set explicitly, use the value set by user; - if
haLitenPort
is not set, assignlistenPort + 1
to it.
…fe set haListenPort value as listenPort +1
51d8d16
@caigy Thank you for your suggestion. I have resubmitted it |
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
[fix] fix6537
After the configuration file is loaded, it was overwritten by the subsequent setHaListenPort
After removing this line of code, the configuration takes effect