Repository navigation
test(core) confluentinc#857: land the probes that measured the revoke path, and the harness they share - #375
Merged
astubbs merged 5 commits intoAug 27, 2026
Conversation
… path, and the harness they share Three probes built during the confluentinc#857 investigation on #29, plus the brokerless harness they share. They pass on master unmodified - which is the point of landing them, not an argument against it. **`OutForProcessingCounterDriftProbeTest` is the instrument that killed a fix.** #29 carried a 42-line revoke-time counter adjustment (`WorkManager.adjustOutForProcessingOnRevoke`) whose whole justification was that `numberRecordsOutForProcessing` drifts high after a rebalance, leaving `isSufficientlyLoaded()` permanently true so the poller never resumes. This probe drives that path and measures the counter. It does not drift, and the adjustment was deleted rather than landed. That is a negative result, and negative results are exactly what gets rediscovered: the reasoning behind the deleted fix is intuitive, the symptom it targeted is real and still open, and nothing in the tree would stop somebody proposing it again next quarter. With the probe on master they have to explain why it is green first. **`ShardManagerStaleContainerTest`** pins that a work container surviving a revoke/reassign cycle does not come back addressable through the shard map - the adjacent assumption the same investigation had to settle before it could attribute anything. **`CloseInterruptLivelockTest`** pins that close does not livelock when the control thread is interrupted mid-shutdown, one of the paths the deadlock hunt kept having to rule out by hand. **`BrokerlessWorkManagerTestBase`** is the harness all three sit on: a `WorkManager` and its collaborators without a broker, so these run in the unit lane in seconds rather than needing Testcontainers. It is deliberately a base class rather than a copied fixture - `docs/testing.md` records what a drifted copy of test setup has already cost this repo once. **No main code is touched.** Extracted from #29 so they can land on their own evidence while that PR remains blocked on its dependencies; #29 will depend on this rather than carry it. Verified on this branch: 6 tests green, `bin/check-all.sh` 15/15, `bin/todo-index.sh --check` clean.
astubbs
marked this pull request as draft
August 26, 2026 15:10
Merged
4 of 6 tasks
astubbs
added a commit
that referenced
this pull request
Aug 26, 2026
… and how to take them back #375 (the three probes and their brokerless harness) and #376 (the back-pressure pause derived from Kafka rather than mirrored) are now open against master, and this PR declares a dependency on both. The content still exists here, so the note says the thing that is easy to get wrong at merge: **take the extracted version, not this branch's copy** - the same rule already recorded for `PCMetrics.java` and #57, for the same reason. The copy that was reviewed on its own merits is the one worth keeping. It also warns against the misreading #376 invites. Its cooperative-pause test passes on master as well as on the change, because master has the mirror but never resets it on assignment - the failure was reachable only on this branch, where the reset was added. The test is a regression guard, not a control arm, and this repo has already paid for reading a non-discriminating test as evidence. The decomposition plan the pieces came from is left alone on purpose. It is a dated record, `docs/citations.md` forbids rewriting those to match today's code, and two of its four clusters have moved since it was written - cluster 3 deleted outright, cluster 4 resolved by deleting the mirror rather than gating it on the eager protocol as the plan proposed. The note now says so, so the plan is read with its corrections beside it rather than being quietly edited into agreement.
…ift-and-stale-container-probes
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Owner
Author
|
@claude review this |
✅ Duplicate Code ReportTwo engines run in parallel for cross-validation. Each has its own thresholds tuned to its baseline - the real safety net is the per-engine "max increase vs base" check. ✅ PMD CPD
No new clones introduced by this PR. ✅ jscpd (language-agnostic)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Claude finished @astubbs's task in 2m 11s —— View job Review
All 4 files are new test-only additions (no main code touched, confirmed by diff). I verified every main-code API these tests call already exists on this branch ( Finding: The class javadoc ( Net effect: the duplication this PR's own javadoc cites as the motivation for extracting a shared base is not actually removed - there are now three near-identical copies of this wiring in the tree (the base class, the untouched
This also touches Everything else looks solid:
No bugs found in the tests' own logic; the one finding above is a documentation/consistency gap between the stated motivation and what the diff actually does, not a correctness defect. |
🧪🔒 Quarantine Lane Report
🔴 expected while the owner PR is open · 🟡🎲 flapper, pass proves nothing · 🚨 a deterministic quarantined test passing means its fix landed: delete its |
|
astubbs
marked this pull request as ready for review
August 27, 2026 03:43
…doc already claimed BrokerlessWorkManagerTestBase was extracted saying two tests had built the same fixture independently, and it named both by role - one that assigns, and EpochAndRecordsMapRaceTest, which must not. Only the first was migrated, so the duplication cited as the motivation was still in the tree and the base's javadoc described a sibling that did not extend it. The review on #375 caught it. Fixed by doing the migration rather than softening the claim: the test drops its own mu/wm/sm/pm/topic/tp and setup(), which were the base's buildWorkManagerFixture() minus the assignment, and inherits them instead. Not assigning is the base's default, so nothing is overridden. That default being silent is the one risk in the move - the deleted setup() carried a comment saying the missing onPartitionsAssigned WAS the race, and inheriting a default loses that. The reasoning is now in the class javadoc as an explicit statement that not overriding assignPartitionsIfWanted() is load-bearing here, so a later reader adding an assignment for convenience has to read why first. docs/refactoring.md carried the wider version of this - four classes opening with the same wiring, noticed on #373. Its entry no longer lists this test as a copier, records that the base now exists and where, and says why the three left are not deletions: ShardAvailableCountOwnershipTest names its topic constant TOPIC, and ShardManagerTest also builds a PartitionState and declares no sm/pm.
…it up twice Both probes in OutForProcessingCounterDriftProbeTest opened with the same MockConsumer, the same options, the same manual rebalance dance and the same gated user function - identical but for one comment, which is what the duplicate-code check reported on #375. Extracted to startGatedPc(), returning the WorkManager the probes then measure. The javadoc on the helper carries the two things that were only implicit while the block was inline, and that a later reader would otherwise be free to "tidy". UNORDERED is not a style choice: it is what allows several records from the single partition to be in flight at once, and that is the state whose accounting these probes exist to measure - under KEY or PARTITION ordering one record is out at a time and the counter is trivially correct, so the probe would pass while measuring nothing. The rebalance/assign/beginning-offsets sequence is MockConsumer's precondition for serving records at all, the same dance MockConsumerTestBase performs. Both probes still pass, so the extraction did not disturb what they measure.
Its first line read "MEASUREMENT PROBE, not intended to merge as-is", which was true while it was an instrument inside #29 and is the opposite of what this PR is for. Left alone it would tell every future reader the file on master was never meant to be there. Rewritten to say what the file is now: the measurement that settled the drift question, kept because green is the RESULT and not a missing assertion. #29 carried a revoke-time counter adjustment justified entirely by a drift this probe does not find, and that adjustment was deleted rather than landed. The javadoc now says so, so the next person to propose it has to explain the green first. Checked for other instances of the class - scaffolding language surviving into a merge - across all four modules' java sources: grep for "not intended to merge", "not for merge", "do not merge", "MEASUREMENT PROBE" and "scratch test" returns this file only.
astubbs
added a commit
that referenced
this pull request
Aug 27, 2026
…e extracted work comes home Fifteen commits, and three of them are this branch's own work returning in the form it was reviewed in: #375 (the probes and their brokerless harness), #376 (the back-pressure pause derived from Kafka), and #267, one of this PR's declared dependencies. #120's PCMetrics work landed with them. **Six of the nine conflicts had one right answer, already written down.** The rule recorded on this branch - take the EXTRACTED version, never this branch's copy, because the extracted one is what got reviewed on its own merits - covers `ThrowableUtils`, `PCMetrics`, `MetricsTeardownCannotBreakCloseTest`, `OutForProcessingCounterDriftProbeTest`, `BrokerPollSystemCooperativeRebalancePauseTest` and the mirror-of-state write-up. All six are now byte-identical to master, so the duplicate copies this branch was carrying are gone rather than merely reconciled. `BrokerPollSystem` is the same call one level down. Master's version is #376 as it landed, refined past what was extracted - a `shouldThrottle` local, a javadoc that explains `TopicPartitionState` rather than asserting the conclusion, and no reference to `ThreadConfinedConsumer`, which is not on master. Taking master's keeps the two from drifting; the cluster-2 note about consumer confinement can be re-added by the change that introduces it, which is where it will be true. **One correction had to be re-applied, and it is worth naming as a pattern.** `pr-blockers-and-collisions.md` was taken from master because master's version is genuinely better - it updates the file-ownership map for #337 and points the reader at `gh pr list` rather than a tally that goes stale at every merge. But master still carries the claim that #29 and #31 target `master-confluent` and must be retargeted, which this branch had already corrected against `gh`: both target `master` and #31 has merged. Taking master's file wholesale reinstated a claim I had already disproved. That is the cost of "take master's version" applied to a file rather than to a change, and the tell was the self-reference gate going red on a line I had fixed once. Re-applied on top of master's version rather than reverting to ours, so both improvements survive. `./mvnw test-compile` green across every module; `bin/check-all.sh` 15/15; `bin/todo-index.sh --check` clean.
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
1 participant
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.
Extracted from #29 so it can land on its own evidence while that PR stays blocked on its dependencies.
Description
Three probes built during the confluentinc#857 investigation, plus the brokerless harness they share. No main code is touched.
They pass on master unmodified. That is the point of landing them rather than an argument against it - each one is the instrument that settled a question, and the answers were negative.
OutForProcessingCounterDriftProbeTestis the probe that killed a fix. #29 carried a 42-line revoke-time counter adjustment (WorkManager.adjustOutForProcessingOnRevoke) whose entire justification was thatnumberRecordsOutForProcessingdrifts high after a rebalance, leavingisSufficientlyLoaded()permanently true so the poller never resumes. This probe drives that path and measures the counter. It does not drift, and the adjustment was deleted rather than landed.That is a negative result, and negative results are what get rediscovered. The reasoning behind the deleted fix is intuitive, the symptom it targeted is real and still open, and nothing in the tree would stop somebody proposing it again next quarter. With the probe on master they have to explain why it is green first.
ShardManagerStaleContainerTestpins that a work container surviving a revoke/reassign cycle does not come back addressable through the shard map - the adjacent assumption the same investigation had to settle before it could attribute anything.CloseInterruptLivelockTestpins that close does not livelock when the control thread is interrupted mid-shutdown, one of the paths the deadlock hunt kept having to rule out by hand.BrokerlessWorkManagerTestBaseis the harness they sit on: aWorkManagerand its collaborators without a broker, so these run in the unit lane in seconds rather than needing Testcontainers. Deliberately a base class rather than a copied fixture -docs/testing.mdrecords what a drifted copy of test setup has already cost this repo.EpochAndRecordsMapRaceTestis on it too. It already existed and had built the same fixture independently, and the base class was extracted naming it - so until this PR the javadoc described a sibling that did not extend it, and the duplication cited as the motivation was still in the tree. The review on this PR caught that; migrating it was the fix rather than softening the claim.docs/refactoring.mdcarries the wider version of the same item and now records which classes are on the base and why the ones left need reconciling rather than deleting.What this does not claim
These are guards, not bug fixes. None of them goes red on master, so none is evidence of a live defect. What they are worth is that the questions they answer stay answered.
Verification
./mvnw -pl parallel-consumer-core surefire:test -Dtest='OutForProcessingCounterDriftProbeTest,ShardManagerStaleContainerTest,CloseInterruptLivelockTest,EpochAndRecordsMapRaceTest'bin/check-all.sh- 15/15bin/todo-index.sh --check- cleanChecklist
N/A - no user-facing behaviour changes; the reasoning is in the commit body and in the probes' own javadocdocs/features/-N/A - tests only, no feature surfacece-simplifyandce-code-reviewlocally -N/A - asked for @claude review on the PR instead. Its finding (the base class's javadoc claiming a migration that had not happened) and the duplicate-code report (the two drift probes standing up the same arrangement) are both fixed on the branch.