Hi Christian,
On 27.10.2025 05:22, Christian Grobmeier wrote:
> Gary effectively added a getter because it helped him; additionally,
> he mentioned that using the path was a good idea anyway.
I agree that adding `Path` support is useful, but there are two issues
with the current proposal in PR #3855:
1. The PR mainly introduces a helper method wrapping
`Paths.get(getFileName())`. In the past Gary has himself rejected
similar PRs to Commons projects that added a single helper without a
compelling use case.
2. The change is applied only to `FileManager`, leaving two other file
managers (`RandomAccessFileManager` and `MemoryMappedFileManager`)
without equivalent support. (Correction to my earlier review [1]:
`RollingFileManager` and `RollingRandomAccessFileManager` would
inherit the method.)
While rewriting the documentation I’ve seen multiple features added to
only *one* file appender simply because it was the one a contributor
happened to use. This inconsistency causes long-term maintenance and
documentation problems. Releasing 2.26.0 with only *partial* `Path`
adoption would feel incomplete.
> The response was "do it during work time". I don't think so: we
> don't ask anyone to pay for changes, we don't force committers to do
> changes during work time. We look at the project's benefits.
It was not my intention to suggest that Gary should only work on this
during his employer’s time. I raised the possibility of funding because
Gary mentioned the change was important to his employer, and I thought
it might be an opportunity to support a broader and more sustainable
effort rather than a minimal change. I clarified this when the topic
came up earlier on `private@` [3].
Gary has a long track record of major OSS contributions [2], and I’m
very aware of the time constraints that come with maintaining multiple
projects. Without that context, I can see how my comment might have been
misunderstood, and I apologize for the confusion.
> Gary explained further, but even a bigger change was requested. I
> know there was no evil intent, but I also see how frustrating this
> can be.
The improvement I suggested in [4] is split into **two steps**, with the
first step being small and achievable (estimated ~8 hours of work). Step
1 can itself be divided into two incremental parts:
**Step 1a:** Convert `fileName` from `String` to `Path` during
configuration, so conversion happens once rather than repeatedly. This
also enables future path validation and security checks.
**Step 1b:** Apply the same change consistently across all file
managers, not just `FileManager`, i.e. include `RandomAccessFileManager`
and `MemoryMappedFileManager`.
> It is not going to work this way, and while we need to keep RTC, we
> need to open up. We have started gatekeeping.
I hear this concern, and I agree we must stay open to contributions. At
the same time, Log4j is a mature project, and small, narrowly scoped
changes can introduce inconsistency if we don’t think about the overall
design. Both Volkan and I support adding `Path` to the API, we just want
to ensure users benefit across all file appenders and that the change is
clearly visible and meaningful in release notes.
> Is a path better than a string? Move forward, merge.
>
> [...]
>
> I'd suggest the following for minor fixes like this:
>
> - If a PR of a committer is opened, check if it harms (format,
> security-wise, or introduces bugs). Accept it as quickly as possible,
> even if you disagree with the long-term goal.
> - If unhappy with the PR, bring the topic up on the dev list again,
> ask for a revert, improvement, or a discussion on the goal
Path is certainly preferable to String, provided it is introduced
consistently. Rather than blocking each other on this PR, I’d like to
suggest a constructive way forward:
1. Gary continues with the public API changes in PR #3855 by:
- Adding `getPath()` consistently across all file manager
implementations.
- Optionally exposing `getPath()` / `setPath()` at the appender level
(`FileAppender`, `FileAppender.Builder`, etc.).
- Agreeing on the final method naming (`getPath()` vs.
`getFilePath()`, etc.).
2. I take care of the internal migration to `Path` by:
- Continuing the refactoring started in PR #3968 [5], which
simplifies passing `Path` from `FileAppender` to `FileManager` and
prepares the codebase for broader NIO adoption.
- Submitting a follow-up PR that replaces internal `String` file
handling with `Path` once PR #3968 is reviewed, keeping changes
focused and easier to evaluate.
Piotr
[1]
https://github.com/apache/logging-log4j2/pull/3855#issuecomment-3113019577
[2] https://github.com/garydgregory
[3] https://lists.apache.org/thread/lzxjkwkbn331tnnz2lyy8fmd9p1mopcf
[4]
https://github.com/apache/logging-log4j2/pull/3855#issuecomment-3113194935
[5] https://github.com/apache/logging-log4j2/pull/3968
