[ISSUE#311] Improve broker register topicrouter info performance#312
[ISSUE#311] Improve broker register topicrouter info performance#312fuyou001 wants to merge 4 commits intoapache:developfrom fuyou001:issues-311
Conversation
|
I understand the purpose of this pull request is to combine topic changes and report them all to nameserver in one registration action. The problem is, each time a new topic is registered, data version of topic config increases and |
fuyou001
changed the title
fuyou001
changed the title
fuyou001
changed the title
|
It makes sense now. Good! |
fuyou001
changed the title
| * If set to 0, newly created topics will be immediately reported to name servers and interval of periodical | ||
| * registration defaults to 10, 000 in milliseconds. | ||
| */ | ||
| private int registerNameServerPeriod = 1000 * 30; |
There was a problem hiding this comment.
registerNameServerPeriod is not a good name, maybe aliveCheckInterval is better. Another, do we need to polish here if no changes in a long time?
There was a problem hiding this comment.
origin default value
| } | ||
| } | ||
| }, 1000 * 10, 1000 * 30, TimeUnit.MILLISECONDS); | ||
| }, 1000 * 10, Math.max(10000, Math.min(brokerConfig.getRegisterNameServerPeriod(), 60000)), TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
How do we prove this is a better value, why not other?
There was a problem hiding this comment.
Name servers will mark the broker as dead and remove routes to this broker if they do not receive registration heartbeat within 2T period. Empirically, brokers should send heartbeats every T moments at most to maintain their aliveness in case of occasional network failure, to tolerate one-time heartbeat loss.
As lite heartbeat mechanism has already been implemented, it's safe to heartbeat more frequently. This is why we allow interval of 10 seconds.
| this.brokerController.registerBrokerAll(false, true, true); | ||
|
|
||
| if (brokerController.getBrokerConfig().getRegisterNameServerPeriod() == 0) { | ||
| this.brokerController.registerBrokerAll(false, true, true); |
There was a problem hiding this comment.
If could not get the topic route info immediately after the topic is created, this may fail some unit tests and confuse some users.
It may be better to detect if the topic route info is changed, and then decide whether it is necessary to register or not.
There was a problem hiding this comment.
We may set the interval to 0 to fix unit tests. Purpose of this pull requests is to group changes and register them all periodically. Users are unlikely to administratively create a topic and send messages immediately. IMO, this change won't bring about much confusion. I won't say no if we keep the default value 0 to maintain behavioral consistency between before and after this pull requests.
There was a problem hiding this comment.
If creating topic is frequenct, it is better to register the just created topic only. In fact, we do not need to change the register protocol, just do something in preparing heartbeat data.
There was a problem hiding this comment.
Following your suggestions, name server has to be updated to accept route data delta type of heartbeat. This would force name server upgrade in deployment prior to deployment of brokers having this patch.
There was a problem hiding this comment.
This results in extra complexities to existing users and deployments.
|
well, Please update your pr template and make your pr description clearly. |
|
This issue is partially merged. In case incremental yet immediate topic registration is required, let's discuss it in a separate thread. |
5 participants
Please do not create a Pull Request without creating an issue first.
What is the purpose of the change
XXXXX
Brief changelog
XX
Verifying this change
XXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[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:checkstyleto make sure basic checks pass. Runmvn clean install -DskipITsto make sure unit-test pass. Runmvn clean test-compile failsafe:integration-testto make sure integration-test pass.