You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@logging.apache.org by Volkan Yazıcı <vo...@gmail.com> on 2021/04/28 13:39:27 UTC
Fix windows build on master (via Tim Perry)
Hello,
Tim has submitted a PR to fix the Windows build on master
<https://github.com/apache/logging-log4j2/pull/469> in February. In
essence, it introduces prepareToStop() to StatusLogger. IIRC, there was a
discussion regarding this subject in the mailing list. I think I have
missed that. The PR looks good to me, though I'd appreciate some more
context, which I've shared in my comments for the PR.
I will appreciate it if somebody else who has more knowledge on
StatusLogger can also skim through the changes. It is not much and should
be straightforward for somebody who has hacked on StatusLogger recently.
Cheers!
Re: Fix windows build on master (via Tim Perry)
Posted by Tim Perry <ti...@gmail.com>.
I created a pull request which fixes the windows build. I have tested on
windows 10 using maven 3.6.3 and Zulu's OpenJDK 11 build and it works. I
noticed the automatic test framework that ran for the pull request failed.
https://github.com/apache/logging-log4j2/pull/491
This request adds an annotation @DisableOnWindows to prevent the
FileOutputTest test from running on Windows in log4j-core. This test cannot
pass because the listener to the StatusLogger is never shut down which
means the file handle to the log file is never released. On Windows, this
means the log file can't be deleted which fails the test.
The PR fixes the XML comparison in ConfigurationBuilderTest in log4j-kafka
to compare the XML as XML documents instead of as Strings which should
prevent the OS dependent quirks from causing the test to fail.
It also fixes an intermittent nuisance in
JsonTemplateLayoutNullEventDelimiterTest which fails when the port is
unavailable on windows. I changed the test to catch the exception and print
an error rather than fail the build. Ideally we'd find an available port
dynamically, write out the XML config to a temp file with the targeted
port, run the test, and then clean up the temporary config XML file. I took
the easy way out here.
I did not update changes.xml because nothing I changed affects the end
user. Is this the correct way to do this?
Please provide feedback on this pull request when it is convenient.
Thanks,
Tim
On Mon, May 3, 2021 at 11:10 PM Tim Perry <ti...@gmail.com> wrote:
> After more carefully reading the code and unit tests, I have come to the
> conclusion that the interaction between context-specific listeners and the
> StatusLogger lifecycle need to be rethought. My approach, at best, would
> have arrived at a kludge that worked but made the code difficult to
> maintain.
>
> As exhibit A, I point you to the method
> configureExistingStatusConsoleListener in StatusConfiguration:
>
> private boolean configureExistingStatusConsoleListener() {
> boolean configured = false;
> for (final StatusListener statusListener :
> this.logger.getListeners()) {
> if (statusListener instanceof StatusConsoleListener) {
> final StatusConsoleListener listener =
> (StatusConsoleListener) statusListener;
> listener.setLevel(this.status);
> this.logger.updateListenerLevel(this.status);
> if (this.verbosity == Verbosity.QUIET) {
> listener.setFilters(this.verboseClasses);
> }
> configured = true;
> }
> }
> return configured;
> }
>
> If the code reconfigures existing StatusConsoleListeners when starting up,
> then we can't very well shut them down when shutting down a context unless
> all contexts that rely on that StatusLogger are shutting down. So my idea
> that I would add the context name to each of the listeners and shut down
> the correct one is flawed because the listener is likely to be shared
> between contexts. Based on this, I have closed the pull request.
>
> I need to take the time to wrap my head around possible lifecycles for the
> StatusListener and StatusConsoleListener objects. I think the LoggerContext
> needs to manage the context-specific StatusLoggers which means the
> StatusConfiguration will need to pass any StatusLogger objects to the
> LoggerContext so it can manage them. As I said before, it is a bit of an
> X-Y problem: I'm trying to fix the file locking but really we need to
> rethink the StatusLogger lifecycle.
>
> I would like to leave my code accessible to you all. Will it remain in the
> closed pull request if I change the underlying branch in GitHub? If not, I
> can create an "bad_ideas" branch and put the code there.
>
> I will carve out the changes I made to
> log4j-kafka\src\test\java\org\apache\logging\log4j\kafka\builder\ConfigurationBuilderTest.java
> to make it platform agnostic and create a pull request for that.
>
> I could also create a pull request for the fixes I had to quell warnings
> if you all are interested. They just deprecated imports and usage of
> constructors for Integer, Long, and Short rather than using the getInteger,
> getLong and getShort static methods.
>
> Please give me your thoughts on:
> 1) managing the StatusLogger lifecycle
> 2) do I need to move my commits from pull 469 to a branch so they remain
> accessible?
> 3) do you want a pull request for the warnings I fixed?
>
> Thanks,
> Tim
>
>
> On Wed, Apr 28, 2021 at 9:06 AM Tim Perry <ti...@gmail.com> wrote:
>
>> I pushed 46f7b08288e1f8843d293bb75950a8b466854ba6 to pull 469.
>>
>> My recollection is that there was a test failing where I thought the
>> problem was the test, not the code. The test was checking to see if the log
>> file was gone inside the @Test and I don't think that is valid. I was
>> meaning to ask someone if the test was incorrect or if my code was the
>> problem.
>>
>> I'll try to get back to this, but I'm slammed with work at the moment.
>>
>>
>> On Wed, Apr 28, 2021 at 8:42 AM Ralph Goers <ra...@dslextreme.com>
>> wrote:
>>
>>> That would be up to you. If you have been working on the same branch
>>> there shouldn’t be a problem. But if it is going to cause merge conflicts
>>> then I wouldn’t bother as you can just link to the prior PR for the history.
>>>
>>> Ralph
>>>
>>> > On Apr 28, 2021, at 7:55 AM, Tim Perry <ti...@gmail.com> wrote:
>>> >
>>> >
>>> >
>>> > It assumes that the status logger is not a singleton. However, status
>>> logger is a singleton and so this is a problem.
>>> >
>>> > I’ve implemented to changes which tracks the different status logger
>>> listeners by the context name. There are still a couple of kinks to work
>>> out on that though. If you want, I can push my changes to GitHub and you
>>> could take a look at those. There is some problem that’s causing some of
>>> the test to fail and I haven’t had time to figure out what the issue is.
>>> >
>>> > Tim
>>> >
>>> >
>>> > Tim Perry
>>> > (916) 505-3634
>>>
>>>
Re: Fix windows build on master (via Tim Perry)
Posted by Tim Perry <ti...@gmail.com>.
After more carefully reading the code and unit tests, I have come to the
conclusion that the interaction between context-specific listeners and the
StatusLogger lifecycle need to be rethought. My approach, at best, would
have arrived at a kludge that worked but made the code difficult to
maintain.
As exhibit A, I point you to the method
configureExistingStatusConsoleListener in StatusConfiguration:
private boolean configureExistingStatusConsoleListener() {
boolean configured = false;
for (final StatusListener statusListener :
this.logger.getListeners()) {
if (statusListener instanceof StatusConsoleListener) {
final StatusConsoleListener listener =
(StatusConsoleListener) statusListener;
listener.setLevel(this.status);
this.logger.updateListenerLevel(this.status);
if (this.verbosity == Verbosity.QUIET) {
listener.setFilters(this.verboseClasses);
}
configured = true;
}
}
return configured;
}
If the code reconfigures existing StatusConsoleListeners when starting up,
then we can't very well shut them down when shutting down a context unless
all contexts that rely on that StatusLogger are shutting down. So my idea
that I would add the context name to each of the listeners and shut down
the correct one is flawed because the listener is likely to be shared
between contexts. Based on this, I have closed the pull request.
I need to take the time to wrap my head around possible lifecycles for the
StatusListener and StatusConsoleListener objects. I think the LoggerContext
needs to manage the context-specific StatusLoggers which means the
StatusConfiguration will need to pass any StatusLogger objects to the
LoggerContext so it can manage them. As I said before, it is a bit of an
X-Y problem: I'm trying to fix the file locking but really we need to
rethink the StatusLogger lifecycle.
I would like to leave my code accessible to you all. Will it remain in the
closed pull request if I change the underlying branch in GitHub? If not, I
can create an "bad_ideas" branch and put the code there.
I will carve out the changes I made to
log4j-kafka\src\test\java\org\apache\logging\log4j\kafka\builder\ConfigurationBuilderTest.java
to make it platform agnostic and create a pull request for that.
I could also create a pull request for the fixes I had to quell warnings if
you all are interested. They just deprecated imports and usage of
constructors for Integer, Long, and Short rather than using the getInteger,
getLong and getShort static methods.
Please give me your thoughts on:
1) managing the StatusLogger lifecycle
2) do I need to move my commits from pull 469 to a branch so they remain
accessible?
3) do you want a pull request for the warnings I fixed?
Thanks,
Tim
On Wed, Apr 28, 2021 at 9:06 AM Tim Perry <ti...@gmail.com> wrote:
> I pushed 46f7b08288e1f8843d293bb75950a8b466854ba6 to pull 469.
>
> My recollection is that there was a test failing where I thought the
> problem was the test, not the code. The test was checking to see if the log
> file was gone inside the @Test and I don't think that is valid. I was
> meaning to ask someone if the test was incorrect or if my code was the
> problem.
>
> I'll try to get back to this, but I'm slammed with work at the moment.
>
>
> On Wed, Apr 28, 2021 at 8:42 AM Ralph Goers <ra...@dslextreme.com>
> wrote:
>
>> That would be up to you. If you have been working on the same branch
>> there shouldn’t be a problem. But if it is going to cause merge conflicts
>> then I wouldn’t bother as you can just link to the prior PR for the history.
>>
>> Ralph
>>
>> > On Apr 28, 2021, at 7:55 AM, Tim Perry <ti...@gmail.com> wrote:
>> >
>> >
>> >
>> > It assumes that the status logger is not a singleton. However, status
>> logger is a singleton and so this is a problem.
>> >
>> > I’ve implemented to changes which tracks the different status logger
>> listeners by the context name. There are still a couple of kinks to work
>> out on that though. If you want, I can push my changes to GitHub and you
>> could take a look at those. There is some problem that’s causing some of
>> the test to fail and I haven’t had time to figure out what the issue is.
>> >
>> > Tim
>> >
>> >
>> > Tim Perry
>> > (916) 505-3634
>>
>>
Re: Fix windows build on master (via Tim Perry)
Posted by Tim Perry <ti...@gmail.com>.
I pushed 46f7b08288e1f8843d293bb75950a8b466854ba6 to pull 469.
My recollection is that there was a test failing where I thought the
problem was the test, not the code. The test was checking to see if the log
file was gone inside the @Test and I don't think that is valid. I was
meaning to ask someone if the test was incorrect or if my code was the
problem.
I'll try to get back to this, but I'm slammed with work at the moment.
On Wed, Apr 28, 2021 at 8:42 AM Ralph Goers <ra...@dslextreme.com>
wrote:
> That would be up to you. If you have been working on the same branch there
> shouldn’t be a problem. But if it is going to cause merge conflicts then I
> wouldn’t bother as you can just link to the prior PR for the history.
>
> Ralph
>
> > On Apr 28, 2021, at 7:55 AM, Tim Perry <ti...@gmail.com> wrote:
> >
> >
> >
> > It assumes that the status logger is not a singleton. However, status
> logger is a singleton and so this is a problem.
> >
> > I’ve implemented to changes which tracks the different status logger
> listeners by the context name. There are still a couple of kinks to work
> out on that though. If you want, I can push my changes to GitHub and you
> could take a look at those. There is some problem that’s causing some of
> the test to fail and I haven’t had time to figure out what the issue is.
> >
> > Tim
> >
> >
> > Tim Perry
> > (916) 505-3634
>
>
Re: Fix windows build on master (via Tim Perry)
Posted by Ralph Goers <ra...@dslextreme.com>.
That would be up to you. If you have been working on the same branch there shouldn’t be a problem. But if it is going to cause merge conflicts then I wouldn’t bother as you can just link to the prior PR for the history.
Ralph
> On Apr 28, 2021, at 7:55 AM, Tim Perry <ti...@gmail.com> wrote:
>
>
>
> It assumes that the status logger is not a singleton. However, status logger is a singleton and so this is a problem.
>
> I’ve implemented to changes which tracks the different status logger listeners by the context name. There are still a couple of kinks to work out on that though. If you want, I can push my changes to GitHub and you could take a look at those. There is some problem that’s causing some of the test to fail and I haven’t had time to figure out what the issue is.
>
> Tim
>
>
> Tim Perry
> (916) 505-3634
Re: Fix windows build on master (via Tim Perry)
Posted by Tim Perry <ti...@gmail.com>.
Should I just push the changes to pull request 469? That way there is a bit
of history.
On Wed, Apr 28, 2021 at 8:26 AM Ralph Goers <ra...@dslextreme.com>
wrote:
> Tim,
>
> Go ahead and create a PR and note that it still has unit test failures
> that need to be looked into. I can’t guarantee one of us will get to it
> before you but at least that way we might be able to.
>
> Ralph
>
> > On Apr 28, 2021, at 7:55 AM, Tim Perry <ti...@gmail.com> wrote:
> >
> > Volkan,
> >
> > I should probably withdraw that pull request.
> >
> > It assumes that the status logger is not a singleton. However, status
> logger is a singleton and so this is a problem.
> >
> > I’ve implemented to changes which tracks the different status logger
> listeners by the context name. There are still a couple of kinks to work
> out on that though. If you want, I can push my changes to GitHub and you
> could take a look at those. There is some problem that’s causing some of
> the test to fail and I haven’t had time to figure out what the issue is.
> >
> > Tim
> >
> >
> > Tim Perry
> > (916) 505-3634
> >
> >> On Apr 28, 2021, at 6:39 AM, Volkan Yazıcı <vo...@gmail.com>
> wrote:
> >>
> >> Hello,
> >>
> >> Tim has submitted a PR to fix the Windows build on master
> >> <https://github.com/apache/logging-log4j2/pull/469> in February. In
> >> essence, it introduces prepareToStop() to StatusLogger. IIRC, there was
> a
> >> discussion regarding this subject in the mailing list. I think I have
> >> missed that. The PR looks good to me, though I'd appreciate some more
> >> context, which I've shared in my comments for the PR.
> >>
> >> I will appreciate it if somebody else who has more knowledge on
> >> StatusLogger can also skim through the changes. It is not much and
> should
> >> be straightforward for somebody who has hacked on StatusLogger recently.
> >>
> >> Cheers!
> >
>
>
>
Re: Fix windows build on master (via Tim Perry)
Posted by Ralph Goers <ra...@dslextreme.com>.
Tim,
Go ahead and create a PR and note that it still has unit test failures that need to be looked into. I can’t guarantee one of us will get to it before you but at least that way we might be able to.
Ralph
> On Apr 28, 2021, at 7:55 AM, Tim Perry <ti...@gmail.com> wrote:
>
> Volkan,
>
> I should probably withdraw that pull request.
>
> It assumes that the status logger is not a singleton. However, status logger is a singleton and so this is a problem.
>
> I’ve implemented to changes which tracks the different status logger listeners by the context name. There are still a couple of kinks to work out on that though. If you want, I can push my changes to GitHub and you could take a look at those. There is some problem that’s causing some of the test to fail and I haven’t had time to figure out what the issue is.
>
> Tim
>
>
> Tim Perry
> (916) 505-3634
>
>> On Apr 28, 2021, at 6:39 AM, Volkan Yazıcı <vo...@gmail.com> wrote:
>>
>> Hello,
>>
>> Tim has submitted a PR to fix the Windows build on master
>> <https://github.com/apache/logging-log4j2/pull/469> in February. In
>> essence, it introduces prepareToStop() to StatusLogger. IIRC, there was a
>> discussion regarding this subject in the mailing list. I think I have
>> missed that. The PR looks good to me, though I'd appreciate some more
>> context, which I've shared in my comments for the PR.
>>
>> I will appreciate it if somebody else who has more knowledge on
>> StatusLogger can also skim through the changes. It is not much and should
>> be straightforward for somebody who has hacked on StatusLogger recently.
>>
>> Cheers!
>
Re: Fix windows build on master (via Tim Perry)
Posted by Tim Perry <ti...@gmail.com>.
Volkan,
I should probably withdraw that pull request.
It assumes that the status logger is not a singleton. However, status logger is a singleton and so this is a problem.
I’ve implemented to changes which tracks the different status logger listeners by the context name. There are still a couple of kinks to work out on that though. If you want, I can push my changes to GitHub and you could take a look at those. There is some problem that’s causing some of the test to fail and I haven’t had time to figure out what the issue is.
Tim
Tim Perry
(916) 505-3634
> On Apr 28, 2021, at 6:39 AM, Volkan Yazıcı <vo...@gmail.com> wrote:
>
> Hello,
>
> Tim has submitted a PR to fix the Windows build on master
> <https://github.com/apache/logging-log4j2/pull/469> in February. In
> essence, it introduces prepareToStop() to StatusLogger. IIRC, there was a
> discussion regarding this subject in the mailing list. I think I have
> missed that. The PR looks good to me, though I'd appreciate some more
> context, which I've shared in my comments for the PR.
>
> I will appreciate it if somebody else who has more knowledge on
> StatusLogger can also skim through the changes. It is not much and should
> be straightforward for somebody who has hacked on StatusLogger recently.
>
> Cheers!