-
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
[Summer of Code] Dledger controller #4195
[Summer of Code] Dledger controller #4195
Conversation
next step: try test
@RongtongJin Hi,take a look please |
common/src/main/java/org/apache/rocketmq/common/namesrv/NamesrvConfig.java
Outdated
Show resolved
Hide resolved
...pache/rocketmq/common/protocol/header/namesrv/controller/AlterSyncStateSetRequestHeader.java
Show resolved
Hide resolved
Some suggestion, details in review:
Describe in Chinese评论中提交了三个建议、适用所有实体定义。请评估。
这些建议的目的均为使得对象安全的创建或者发布,避免可以获取到对象引用或者对象集合字段引用的不可信代码、不恰当的使用导致对象状态被破坏。 |
.../src/main/java/org/apache/rocketmq/common/protocol/header/namesrv/controller/ErrorCodes.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/ControllerRequestProcessor.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/ControllerRequestProcessor.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/processor/ControllerRequestProcessor.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/manager/ReplicasInfoManager.java
Outdated
Show resolved
Hide resolved
if (!brokerIdTable.containsKey(brokerAddress)) { | ||
// If this broker replicas is first time come online, we need to apply a new id for this replicas. | ||
brokerId = brokerInfo.newBrokerId(); | ||
final ApplyBrokerIdEvent applyIdEvent = new ApplyBrokerIdEvent(request.getBrokerName(), | ||
brokerAddress, brokerId); | ||
result.addEvent(applyIdEvent); |
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.
Will generating events based on the values in memory cause duplicate brokerId or epoch?
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.
Actually not, because eventScheduler is scheduled in fifo order.
The next eventHandler will be dispatched only after the events generated by the previous eventHandler are appended to the dledger and applied to the state machine.
So it doesn't happen as you said.
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.
这需要依靠 dledger snapshot 的支持, 从最近一次 snapshot 恢复状态, 然后恢复之后的日志.
但是目前 dledger 不支持该能力, 所以只能从头开始回放日志.
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.
讨论方案: 在 role change 到 leader 时, 主动发起一次空的提案. 等该提案提交之后, 才代表controller 的元数据都已经恢复了.然后才能对外提供服务.
common/src/main/java/org/apache/rocketmq/common/namesrv/NamesrvConfig.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/impl/DledgerController.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/NamesrvController.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/NamesrvController.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/Controller.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/manager/ReplicasInfoManager.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/manager/ReplicasInfoManager.java
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/manager/InSyncReplicasInfo.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/manager/InSyncReplicasInfo.java
Outdated
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/manager/ReplicasInfoManager.java
Outdated
Show resolved
Hide resolved
@RongtongJin Thanks a lot |
tryTimes++; | ||
if (tryTimes > 3) { | ||
log.error("Controller leader append initial log failed too many times"); | ||
break; | ||
} |
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.
所以连续超过3次会跳出循环吗?是不是应该循环直到append成功
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.
可能刚转换成 leader 的时候, 需要一段时间才能 append 成功. 但是并不知道成功的边界. 会不会有可能一直不成功?
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.
是的,但我认为成功前不应该提供服务,或者每失败超过x次打一次日志来提醒用户。
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.
good idea
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/impl/DledgerController.java
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/impl/DledgerController.java
Show resolved
Hide resolved
namesrv/src/main/java/org/apache/rocketmq/namesrv/controller/manager/ReplicasInfoManager.java
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## 5.0.0-beta-dledger-controller #4195 +/- ##
===================================================================
+ Coverage 43.13% 43.28% +0.15%
- Complexity 6025 6138 +113
===================================================================
Files 795 818 +23
Lines 56859 57559 +700
Branches 7787 7852 +65
===================================================================
+ Hits 24524 24917 +393
- Misses 29141 29404 +263
- Partials 3194 3238 +44
Continue to review full report at Codecov.
|
What is the purpose of the change
tracking issue: #4330
Add a controller for ha-service, the controller is based on dledger.
The architecture is:
DledgerController 里面有几个组件, 他们的关系是这样的:
1.不会修改状态机的, 例如 alterSyncStateSet 和 electMaster 等, 这些函数会被 controller 所调用, 根据内存中的元数据, 生成一些 event.
然后 controller 会将这些 event 包装成 eventHandler, 投放到 EventScheduler的队列, 由 eventHandler append 到dledger.
2.会修改状态机的, 例如 handleXXXX (handleElectMaster), 这是由 ControllerStateMachine 调用的. (也即当 event 成功 append 到 dledger 后, 就会调用相应的 handleXXX 函数, 修改内存中的元数据).
Brief changelog
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily. Notice,
it would be helpful if you could finish the following 5 checklist(the last one is not necessary)before request the community to review your PR
.[ISSUE #123] Fix UnknownException when host config not exist
. Each commit in the pull request should have a meaningful subject line and body.mvn -B clean apache-rat:check findbugs:findbugs checkstyle:checkstyle
to make sure basic checks pass. Runmvn clean install -DskipITs
to make sure unit-test pass. Runmvn clean test-compile failsafe:integration-test
to make sure integration-test pass.