[ISSUE #9881] Increase broker notification timeout

[Aias00]

Which Issue(s) This PR Fixes

Fixes #9881

Brief Description

Increase the timeout for NOTIFY_MIN_BROKER_ID_CHANGE broker notification requests from 300ms to 3000ms. This gives the NameServer broker notification path the timeout requested in the issue while keeping the change scoped to that notification.

How Did You Test This Change?

• mvn -pl namesrv -Dtest=RouteInfoManagerNewTest#notifyMinBrokerIdChangedUsesBrokerNotificationTimeout test -DfailIfNoTests=false -Djacoco.skip=true -Dspotbugs.skip=true -Dcheckstyle.skip=true -Drat.skip=true
• mvn -pl namesrv -Dtest=RouteInfoManagerNewTest test -DfailIfNoTests=false -Djacoco.skip=true -Dspotbugs.skip=true -Dcheckstyle.skip=true -Drat.skip=true
• git diff --check

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[RockteMQ-AI]

Review by github-manager-bot

Summary

This PR increases the timeout for NOTIFY_MIN_BROKER_ID_CHANGE broker notification requests from 300ms to 3000ms, replacing a hardcoded magic number with a named constant NOTIFY_MIN_BROKER_ID_CHANGE_TIMEOUT_MILLIS. A new unit test verifies the timeout value via reflection.

Findings

• [Info] RouteInfoManager.java:71 — The named constant improves readability and maintainability. The 10× increase (300ms → 3000ms) directly addresses issue #9881.

• [Info] RouteInfoManager.java:935 — The notifyMinBrokerIdChanged method iterates over brokers sequentially in a loop. With a 3000ms timeout per broker, worst-case blocking time is N × 3000ms where N is the number of brokers to notify. For large clusters this could become noticeable. Consider whether async/parallel notification would be a future improvement — not a blocker for this PR.

• [Info] RouteInfoManagerNewTest.java — The test uses reflection (setAccessible(true)) to invoke the private notifyMinBrokerIdChanged method. This is a common Java testing pattern but makes the test fragile to method signature changes. Consider making the method package-private if the team is comfortable with that.

Verdict

Clean, well-scoped change that correctly addresses the referenced issue. The code is straightforward, the constant extraction is good practice, and the test coverage is appropriate.

---

Automated review by github-manager-bot

[RockteMQ-AI]

Review by github-manager-bot

Summary

Extracts the hardcoded 300ms timeout for notifyMinBrokerIdChange into a named constant NOTIFY_MIN_BROKER_ID_CHANGE_TIMEOUT_MILLIS and increases it to 3000ms.

Findings

• [Warning] RouteInfoManager.java:935 — The timeout increase from 300ms to 3000ms (10x) is significant. While the intention is to prevent premature timeouts during broker notification, a 3-second blocking timeout in the NameServer's route info update path could slow down broker registration propagation under normal conditions. Consider:
  1. Whether the root cause is network latency or broker-side processing delay
  2. If this should be configurable via namesrvConfig rather than a static constant
  3. Whether invokeOneway actually blocks for the full timeout or just sets a write timeout
• [Info] RouteInfoManager.java:71 — Good practice to extract magic numbers into named constants. The naming convention is consistent with the codebase.
• [Info] Test coverage is added for the new timeout value.

Suggestions

Consider making the timeout configurable:

private final long notifyMinBrokerIdChangeTimeoutMillis = 
    namesrvController.getNamesrvConfig().getNotifyMinBrokerIdChangeTimeoutMillis();

Verdict

The fix addresses a real issue (300ms being too aggressive), but the 3000ms value should ideally be configurable rather than hardcoded.

---

Automated review by github-manager-bot

[Aias00]

Addressed the review feedback in 0087ac958:

• Moved the broker notification timeout into NamesrvConfig as notifyMinBrokerIdChangeTimeoutMillis with the same 3000ms default.
• Updated RouteInfoManager to read the timeout from namesrv config instead of a static constant.
• Extended the existing test to verify both the default timeout and a configured override value.

Local verification:
mvn -pl namesrv -am -Dtest=RouteInfoManagerNewTest#notifyMinBrokerIdChangedUsesBrokerNotificationTimeout -DfailIfNoTests=false test passed; git diff --check passed.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.24%. Comparing base (577b89f) to head (0087ac9).
⚠️ Report is 2 commits behind head on develop.

Files with missing lines	Patch %	Lines
.../apache/rocketmq/common/namesrv/NamesrvConfig.java	0.00%	4 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10653      +/-   ##
=============================================
- Coverage      48.32%   48.24%   -0.09%     
+ Complexity     13490    13481       -9     
=============================================
  Files           1380     1380              
  Lines         101031   101096      +65     
  Branches       13092    13101       +9     
=============================================
- Hits           48825    48769      -56     
- Misses         46247    46345      +98     
- Partials        5959     5982      +23     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[RockteMQ-AI]

Review by github-manager-bot (Re-review after new commit)

Summary

The author addressed the previous review suggestion: the timeout is now configurable via NamesrvConfig.getNotifyMinBrokerIdChangeTimeoutMillis() instead of being a hardcoded constant. Default value remains 3000ms.

Findings

• [Info] NamesrvConfig.java — Clean addition of notifyMinBrokerIdChangeTimeoutMillis field with getter/setter. Follows existing config patterns.
• [Info] RouteInfoManager.java:934 — Now reads timeout from config: this.namesrvConfig.getNotifyMinBrokerIdChangeTimeoutMillis(). This is exactly the configurable approach suggested in the previous review.
• [Info] Test verifies both the default (3000ms) and a custom value (1234ms), confirming configurability works correctly.

Verdict

The previous concern about hardcoding is fully resolved. The change is clean, well-tested, and backward-compatible (default 3000ms matches the previous intent). LGTM.

---

Automated re-review by github-manager-bot