[ISSUE #10425] Revert split registration dataVersion change#10426
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10426 +/- ##
=============================================
- Coverage 48.05% 47.95% -0.11%
+ Complexity 13303 13277 -26
=============================================
Files 1377 1377
Lines 100611 100613 +2
Branches 12991 12992 +1
=============================================
- Hits 48347 48246 -101
- Misses 46348 46427 +79
- Partials 5916 5940 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
oss-sentinel-ai
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Restores the DataVersion.nextVersion() advancement in TopicConfigManager#buildSerializeWrapper when enableSplitRegistration is enabled, fixing the issue where later split registration batches skip topic config updates on the NameServer.
Findings
-
[Correctness] ✅ The fix is minimal and precisely scoped. The
nextVersion()call is correctly guarded byisEnableSplitRegistration(), so non-split registration is unaffected. -
[Root cause verified] ✅ Traced through
RouteInfoManager#registerBroker→isBrokerTopicConfigChanged(comparesDataVersionfrom each split batch against stored value frombrokerLiveTable). Without per-batchDataVersionadvancement, the NameServer stores the version after the first batch and subsequent batches appear unchanged, causingisTopicConfigChangedto returnfalsefor existing topics. -
[Tests] ✅
testBuildSerializeWrapperUpdatesDataVersionWhenSplitRegistrationEnableddirectly verifies the counter increment. Good regression test coverage for the specific fix. -
[CI] ✅ All 10 checks pass (CodeQL, bazel-compile, maven-compile × 4 platforms, coverage, license, misspell).
-
[Compatibility] ✅ No API or protocol changes. No backward compatibility concerns.
Suggestions
Consider adding a second assertion in the test to also verify the DataVersion stored in the returned wrapper matches the counter, confirming the wrapper carries the incremented version — this would more closely mirror the actual runtime check in RouteInfoManager:
// Already present, good:
Assert.assertEquals(counterAfter, wrapper.getDataVersion().getCounter().get());Actually this assertion already exists — well done. LGTM.
Automated review by github-manager-bot
Which Issue(s) This PR Fixes
Brief Description
This reverts #9521 and restores the topic config
DataVersionadvancement whenenableSplitRegistrationis enabled.With split broker registration, a broker sends topic configs to the name server in multiple registration requests. The name server records the broker
DataVersionafter each request and uses it to decide whether existing topic route data should be updated. If all split requests carry the sameDataVersion, later batches can be treated as unchanged for topics that already exist under the broker.Restoring the per-wrapper
DataVersionadvancement keeps each split registration batch visible as a distinct update. This PR also adds a focused regression test forTopicConfigManager#buildSerializeWrapper.How Did You Test This Change?
/usr/local/bin/mvn -pl broker -am -Dtest=TopicConfigManagerTest -DfailIfNoTests=false -DskipITs -DskipCheckStyle test