oscerd commented on PR #26734: URL: https://github.com/apache/camel/pull/26734#issuecomment-5797876384
Rebased onto current `main` and pushed `cf32ce93`. The branch was 30 commits behind, and [CAMEL-24911](https://issues.apache.org/jira/browse/CAMEL-24911) had since added abstract methods to `ErrorRegistry`, so `camel-base-engine` no longer compiled against a current `camel-api` locally. Cherry-picked onto a fresh branch rather than rebasing, since a shallow clone has no merge base and replays unrelated history. ### @davsclaus Thanks for going back and grepping — and for saying so plainly. That is the second review this week to cite that rule, so correcting it in public is worth more than the four suggestions were. Your open question about a permanent home for the two properties still stands and I have deliberately left it alone. My read is the same as yours: the upgrade guide is migration-only, so after 4.23 these are discoverable only by grepping a document nobody re-reads. Two candidates, both structural enough to belong to a committer rather than to this branch — a new resource-resolution section in the user manual, or folding them into the properties/configuration reference next to the other ad-hoc `camel.*` keys. Say which and I will open a separate docs issue. ### @gnodet — the `stopped` ordering Applied, but not for the stated reason, and I want to be accurate about what I found rather than just take the suggestion. Under the default `PER_METHOD` lifecycle this is not a race. JUnit builds a fresh test instance per method, so `stopped` is already `false` before the acceptor starts; the assignment is redundant, not late. `PER_CLASS` is the case where it could bite, because `@AfterEach` leaves the field `true` for the next test. So I tried to make it fail: lifecycle forced to `PER_CLASS`, and `@TestMethodOrder(MethodName.class)` so that `getInputStreamGivesUpOnAServerThatNeverAnswers` — the only test asserting on the accept latch — runs fourth rather than first, which is the ordering that would expose a stale flag. **All five still passed.** `Thread.start()` takes far longer than the single volatile store that followed it, so the main thread wins that window in practice. I am applying it anyway: the assignment belongs before the thread that reads it, it costs one line, and a later lifecycle change would make the hazard real. But it is a clarity change, not a fix for anything that was breaking, and the description "race condition that can silently break the mute-server tests" overstates what is there. Failing to reproduce is not proof of impossibility and I am not claiming it is — the window genuinely exists under `PER_CLASS`, it is just far too narrow to hit. ### Test visibility Settled above by the reviewer who raised it: the rule is not in `CLAUDE.md` or `.oss-ai-helper-rules/`, and `core/camel-core` is 2725 public test classes to 112 package-private. Leaving them `public` to match the module. 22 tests green on the rebased branch (17 `ResourceHelperTest` + 5 new). _Claude Code on behalf of @oscerd_ -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
