lizhimins commented on PR #5067:
URL:
https://github.com/apache/rocketmq-dashboard/pull/5067#issuecomment-5845589506
Thanks for the patch. We are closing this one, and none of the reasons are
about the three-line change itself, which is correct and harmless.
1. There is no reachable trigger. `awaitJob` only ever sees futures produced
by `completionService.submit(...)` (`CollectorScheduler.java:176`) on an
`ExecutorCompletionService` (`:127`), i.e. JDK `FutureTask`s, and
`FutureTask.report()` throws `new ExecutionException(cause)` where `cause` is
the throwable recorded by `setThrowable` — never null. Your description
concedes this ("Although FutureTask normally sets a non-null cause") and then
argues from `new ExecutionException(null)` being legal, which is an API-shape
argument rather than a path through this code.
2. The second test does not test anything.
`awaitJobLogsCauseMessageWhenExecutionExceptionHasCauseTest` has no assertion,
no log appender and no `verify`, so it passes identically with and without the
fix; the description ("verifies the cause's message is logged") does not match
the diff. The null-cause case does have teeth, but only because the reflective
invoke rethrows the NPE.
3. Both cases reach a private record and a private method through
`Class.forName` / `setAccessible` (`CollectionJob` at `:435`, `awaitJob` at
`:215`), a pattern this file has never used — all 12 existing cases drive the
public `collect()` and assert on persisted samples. They also stub
`lease.tryAcquire()`, which `awaitJob` never reads.
4. The commit subject `fix collectorScheduler problem` is not Conventional
Commits (`CONTRIBUTING.md:106-108`).
If you want to pursue the hardening, reframe it as API-contract hardening
rather than a live NPE, drive it through `collect()` with a collector mock that
throws, and assert the logged message with a `ListAppender`.
--
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]
