-
Notifications
You must be signed in to change notification settings - Fork 11.7k
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 #6662] Optimize the process of HA's confirmOffset calculation #6663
Conversation
…all connect to the master.
@@ -268,6 +268,7 @@ public void changeToMaster(final int newMasterEpoch, final int syncStateSetEpoch | |||
|
|||
// Notify ha service, change to master | |||
this.haService.changeToMaster(newMasterEpoch); | |||
this.haService.setBrokerControllerId(this.brokerControllerId); |
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.
After registration is completed, the brokerControllerId is determined, and there is no need to set it every time the master is switched.
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.
OK, I'll modify this logic.
Codecov Report
@@ Coverage Diff @@
## develop #6663 +/- ##
=============================================
- Coverage 43.11% 43.02% -0.10%
- Complexity 8999 9019 +20
=============================================
Files 1107 1109 +2
Lines 78343 78591 +248
Branches 10217 10226 +9
=============================================
+ Hits 33775 33810 +35
- Misses 40336 40552 +216
+ Partials 4232 4229 -3
... and 31 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@@ -432,7 +434,7 @@ private long computeConfirmOffset() { | |||
long confirmOffset = this.defaultMessageStore.getMaxPhyOffset(); |
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.
should refactor confirmOffset
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.
done, it has been renamed to newConfirmOffset.
@@ -600,6 +600,7 @@ private void confirmNowRegisteringState() { | |||
if (this.brokerMetadata.isLoaded()) { | |||
this.registerState = RegisterState.CREATE_METADATA_FILE_DONE; | |||
this.brokerControllerId = brokerMetadata.getBrokerId(); | |||
this.haService.setBrokerControllerId(this.brokerControllerId); |
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 it is the first time to start (without brokerIdentity file), the brokerControllerId will be forgotten to be set to HAService.
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.
Yes, you are right.
I have modify the code to set brokerControllerId when it starts without brokerIdentity file.
… without brokerIdentity file.
Which Issue(s) This PR Fixes
Fixes #6662
Brief Description
Determine whether all brokers in syncStateSet have established connections. If not, the confirmOffset cannot be updated.
判断syncStateSet中的broker是否都建立有连接。如果没有,则无法更新confirmOffset.
How Did You Test This Change?
using openchaos.
So far no messages have been lost.