Promote validator registry reset() to public lifecycle API and invoke from controller stop#18240
Merged
xiangfu0 merged 1 commit intoApr 17, 2026
Conversation
… from controller stop TableConfigValidatorRegistry and InstanceConfigValidatorRegistry held validators in static lists with only a @VisibleForTesting reset() for cleanup. In production a controller restart is a JVM restart, so the leak is invisible; but integration tests that restart the controller in-process (ControllerTest.restartController()) accumulate one validator per restart, and the older ones retain references to a now-disconnected HelixAdmin. The next config-mutation request then dereferences torn-down Helix state and rejects the write with HTTP 400. Changes: - Promote reset() to a public lifecycle API on both registries (drop @VisibleForTesting). Document the lifecycle contract in class Javadoc. - Add unregister(validator) on both registries for API completeness. - Invoke TableConfigValidatorRegistry.reset() and InstanceConfigValidatorRegistry.reset() from BaseControllerStarter.stop() so the cleanup is automatic for any subclass that registered validators. This is additive: existing callers are unaffected, production behavior is unchanged (stop() = JVM exit), and only in-process restart scenarios observe a behavior difference — validators no longer leak across restarts.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #18240 +/- ##
============================================
- Coverage 63.40% 63.38% -0.03%
Complexity 1627 1627
============================================
Files 3243 3243
Lines 197162 197167 +5
Branches 30495 30495
============================================
- Hits 125009 124970 -39
- Misses 62151 62193 +42
- Partials 10002 10004 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xiangfu0
approved these changes
Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
reset()onTableConfigValidatorRegistryandInstanceConfigValidatorRegistryfrom a@VisibleForTestinghelper to a public lifecycle API. Addedunregister(validator)on both registries for API completeness.TableConfigValidatorRegistry.reset()andInstanceConfigValidatorRegistry.reset()fromBaseControllerStarter.stop()so cleanup is automatic for any subclass that registered validators.Motivation
The validator SPI added in #18167 holds validators in static
CopyOnWriteArrayLists. In production this is invisible (controller restart = JVM restart). In integration tests that restart the controller in-process — e.g.ControllerTest.restartController()— eachstart()registers a new validator, but the previous validator stays in the static list and retains references to a now-disconnectedHelixAdmin. After N restarts, the oldest stale validator throws when the next config-mutation request iterates it, and the write is rejected with HTTP 400.The fix is additive:
stop()previously coincided with JVM exit, which already cleared the static registry. Invokingreset()explicitly only affects in-process restart scenarios (integration tests, downstream distributions that restart controllers without JVM restart).Test plan
./mvnw -pl pinot-spi -am test -Dtest='TableConfigValidatorRegistryTest,InstanceConfigValidatorRegistryTest'./mvnw -pl pinot-spi spotless:apply checkstyle:check license:format license:check./mvnw -pl pinot-controller compile./mvnw -pl pinot-controller spotless:apply checkstyle:check license:format license:check