SEZ9 commented on PR #11983:
URL: https://github.com/apache/seatunnel/pull/11983#issuecomment-5469331876
Thanks @DanielLeens — the explanation of how the diff shrank makes sense:
with the functional half carried onto `dev` by the #11984 squash merge (via
`1bf67a91b`), leaving only the javadoc correction at `d583444bc2`, the reduced
scope of this PR checks out.
On the change itself: the reworded sentence ("clears the logger's explicit
level rather than leaving it unchanged") is the accurate description, and
aligning it with the explanation already in `LogLevels.parse`'s javadoc on
`dev` is the right call. No concerns with the doc-only diff as written.
That said, since the functional code from my earlier review rounds is now on
`dev` rather than in this PR, the open findings against it still need a home.
Concretely:
- **F1 (HIGH)**: the blank-logger guard in `Log4j2HttpPostCommandProcessor`
rejects `LoggerConfig.ROOT` (empty string) with 400 before `LogLevels.apply`
can reach `setRootLevel`, so setting the root logger level via the endpoint is
impossible.
- **F2**: the logger name is validated with `trim()` but applied untrimmed,
so whitespace-padded names still silently no-op.
- **F3**: `LogLevels.parse` rejects valid mixed-case custom log4j2 levels
(e.g. one registered as `Notice`).
- **F4**: `params[2]`/`params[3]` are indexed without a local length guard.
- **F5**: `LogLevels.apply` relies on callers to reject null levels — the
javadoc fix here describes the hazard, but the code path itself is still
caller-dependent.
- **F6–F8**: reflected raw `levelName` in the 400 body,
`LogLevelsTest.restoreLevel` materializing an explicit `LoggerConfig` in the
shared context, and the `validNames()` "most severe first" claim vs.
unspecified `Level.values()` ordering.
My asks:
1. Please confirm whether you plan a follow-up PR against `dev` for F1–F5
(F1 in particular, since it's a functional regression in the merged code), or
whether you'd prefer to expand this PR back to include those fixes.
2. If a follow-up is the plan, F6–F8 can ride along there too — just let me
know so I can track them against the right PR.
Once that's settled, this javadoc change itself is fine to merge.
<!-- streview-comment:684 -->
--
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]
