Use RTC in Log4j and Log4Net

[Piotr P. Karwasz]

Hi all,

Inspired by the way the Log4Net team works, I would like to propose a
switch from our CTR policy to RTC:

* every change to the main branches should go through a pull request,
* pull requests can be merged if they have 1 positive review and no
requested changes. To allow everyone to look at the PR, let's not
merge them before at least 24 hours have passed.
* if a pull request does not receive any review in 72 hours, as a last
resort we merge them without a review.

I would like to apply this policy to our most active repos:

* l-log4j2
* l-log4j-kotlin
* l-log4j-scala
* l-log4j-transform
* l-log4j-tools
* l-log4net

I am NOT proposing this change to the equally active l-log4cxx
repository, since the Log4cxx team has an established workflow that
does not use formal reviews.

I would prefer to explicitly add a branch protection rule that
requires reviews. If a PR is not reviewed within 72 hours, please ping
the Slack channel so someone can review it.

What do you think?

Piotr

Re: Can't build log4j main branch

[Piotr P. Karwasz]

Hi Volkan,

On Mon, 16 Sept 2024 at 20:19, Volkan Yazıcı  wrote:
> Gary has a failure on L361, that is, it retries every 100ms to succeed with
> `logger.info()` for at most 2mins. I doubt if more waiting will solve the
> problem. I tried to improve that test several times (see its history), but
> Windows just behaves weird with sockets. I'd appreciate it if Windows users
> can share some tips on what else we can do.

IIRC Windows has a very rude firewall: if the connection to a port is
allowed, but the port is closed, the firewall drops all packages
instead of reporting an appropriate error.
Maybe we should add more logging to the reconnector? For example it
would be probably appropriate to log all connection errors at the
`WARN` level and all connection attempts at the `INFO` level.

> Piotr, you say "a proven history of flakiness", but the link you shared
> states, out of 1.26k times, in the last 90 days, it has failed only 2
> times, and was flaky only 16 times. What is our definition of flakiness
> here?

For me a test is either flaky or not. The SocketAppenderReconnectTest
is a very good test (especially when compared with the `RollingFile`
appender tests), but it is still missing some synchronisation.
BTW I have also tried to improve it last year, but some flakiness remains.

Piotr

Re: Use RTC in Log4j and Log4Net

[Ralph Goers]

Why? i.e. - what currently isn’t working?

Ralph

> On Sep 17, 2024, at 1:30 AM, Piotr P. Karwasz  wrote:
> 
> Hi all,
> 
> Inspired by the way the Log4Net team works, I would like to propose a
> switch from our CTR policy to RTC:
> 
> * every change to the main branches should go through a pull request,
> * pull requests can be merged if they have 1 positive review and no
> requested changes. To allow everyone to look at the PR, let's not
> merge them before at least 24 hours have passed.
> * if a pull request does not receive any review in 72 hours, as a last
> resort we merge them without a review.
> 
> I would like to apply this policy to our most active repos:
> 
> * l-log4j2
> * l-log4j-kotlin
> * l-log4j-scala
> * l-log4j-transform
> * l-log4j-tools
> * l-log4net
> 
> I am NOT proposing this change to the equally active l-log4cxx
> repository, since the Log4cxx team has an established workflow that
> does not use formal reviews.
> 
> I would prefer to explicitly add a branch protection rule that
> requires reviews. If a PR is not reviewed within 72 hours, please ping
> the Slack channel so someone can review it.
> 
> What do you think?
> 
> Piotr

Re: Use RTC in Log4j and Log4Net

[Piotr P. Karwasz]

Hi Ralph,

On Tue, 17 Sept 2024 at 15:47, Ralph Goers  wrote:
>
> Why? i.e. - what currently isn’t working?

I merely wish to formalize what is already happening and set up a
branch protection rule to enforce it.

Note that I have never seen a PR in Log4Net being merged without a review.

On the other hand you can probably find a couple of my recent PRs that
don't have a formal review. Sure, I must have certainly consulted with
someone on Slack before I merged them, but there is no sign of it.
Let's make it formal, so users also see it.

Piotr

Re: Use RTC in Log4j and Log4Net

[Gary Gregory]

Maybe we should talk about net vs. J separately?

Gary

On Tue, Sep 17, 2024, 10:53 AM Piotr P. Karwasz 
wrote:

> Hi Ralph,
>
> On Tue, 17 Sept 2024 at 15:47, Ralph Goers 
> wrote:
> >
> > Why? i.e. - what currently isn’t working?
>
> I merely wish to formalize what is already happening and set up a
> branch protection rule to enforce it.
>
> Note that I have never seen a PR in Log4Net being merged without a review.
>
> On the other hand you can probably find a couple of my recent PRs that
> don't have a formal review. Sure, I must have certainly consulted with
> someone on Slack before I merged them, but there is no sign of it.
> Let's make it formal, so users also see it.
>
> Piotr
>

[VOTE][LAZY] Release Apache Logging Parent `11.3.0` (RC1)

[Piotr P. Karwasz]

This is a lazy-vote to release the Apache Logging Parent `11.3.0`.

Website: https://logging.staged.apache.org/logging-parent-11.3.0
GitHub: https://github.com/apache/logging-parent
Commit: 47c5a7a1073ac75ebb8aee58277a6b574862a990
Distribution: 
https://dist.apache.org/repos/dist/dev/logging/logging-parent/11.3.0
Nexus: https://repository.apache.org/content/repositories/orgapachelogging-1297
Signing key: 0x077e8893a6dcc33dd4a4d5b256e73ba9a0b592d0
Review kit: 
https://logging.apache.org/logging-parent/release-review-instructions.html

Please download, test, and cast your votes on this mailing list.

[ ] +1, release the artifacts
[ ] -1, don't release, because...

This vote is open for 72 hours and will pass unless getting a
net negative vote count. All votes are welcome and we encourage
everyone to test the release, but only the Logging Services PMC
votes are officially counted.

== Release Notes

This minor release migrates the BeanShell distribution archive script to Groovy.

The update to the Develocity extensions should fix the lack of test
results with Maven Surefire 3.5.0.

=== Changed

* Switch the distribution script from BeanShell to Groovy. (#196)

=== Fixed

* Fix race condition, which marks merged Dependabot PRs as closed.

=== Updated

* Update `actions/setup-java` to version `4.3.0` (#237)
* Update `actions/upload-artifact` to version `4.4.0` (#233)
* Update `com.github.spotbugs:spotbugs-maven-plugin` to version `4.8.6.3` (#235)
* Update `com.google.errorprone:error_prone_core` to version `2.32.0` (#240)
* Update `com.gradle:common-custom-user-data-maven-extension` to
version `2.0.1` (#236)
* Update `com.gradle:develocity-maven-extension` to version `1.22.1` (#239)
* Update `github/codeql-action` to version `3.26.7` (#243)
* Update `org.apache.groovy:groovy` to version `4.0.23` (#244)
* Update `org.eclipse.jgit:org.eclipse.jgit` to version
`7.0.0.202409031743-r` (#238)

Re: Use RTC in Log4j and Log4Net

[Ralph Goers]

First, the obvious. I haven’t committed much in a while. The last several I did 
I used PRs primarily because it makes it easier for people to review the 
changes but I didn’t necessarily wait for a review.  For really simple stuff 
I've never use a PR. However, with the switch from Jira to GitHub issues it 
might make more sense to use a PR since you have to have something to track the 
problem. But if there is already an issue then I would probably just use that. 
Again, depending on what is being done.

I wouldn’t classify any of the work I’ve seen you doing recently as trivial or 
minor, so you only doing PRs makes sense. 

Ralph

> On Sep 17, 2024, at 7:52 AM, Piotr P. Karwasz  wrote:
> 
> Hi Ralph,
> 
> On Tue, 17 Sept 2024 at 15:47, Ralph Goers  wrote:
>> 
>> Why? i.e. - what currently isn’t working?
> 
> I merely wish to formalize what is already happening and set up a
> branch protection rule to enforce it.
> 
> Note that I have never seen a PR in Log4Net being merged without a review.
> 
> On the other hand you can probably find a couple of my recent PRs that
> don't have a formal review. Sure, I must have certainly consulted with
> someone on Slack before I merged them, but there is no sign of it.
> Let's make it formal, so users also see it.
> 
> Piotr

Re: Use RTC in Log4j and Log4Net

[Matt Sicker]

I’m -1 on switching to RTC. Same reason as always. Losing momentum from waiting 
for an unnecessary code review will simply lead to much longer gaps between 
time I spend on the project.

> On Sep 17, 2024, at 03:30, Piotr P. Karwasz  wrote:
> 
> Hi all,
> 
> Inspired by the way the Log4Net team works, I would like to propose a
> switch from our CTR policy to RTC:
> 
> * every change to the main branches should go through a pull request,
> * pull requests can be merged if they have 1 positive review and no
> requested changes. To allow everyone to look at the PR, let's not
> merge them before at least 24 hours have passed.
> * if a pull request does not receive any review in 72 hours, as a last
> resort we merge them without a review.
> 
> I would like to apply this policy to our most active repos:
> 
> * l-log4j2
> * l-log4j-kotlin
> * l-log4j-scala
> * l-log4j-transform
> * l-log4j-tools
> * l-log4net
> 
> I am NOT proposing this change to the equally active l-log4cxx
> repository, since the Log4cxx team has an established workflow that
> does not use formal reviews.
> 
> I would prefer to explicitly add a branch protection rule that
> requires reviews. If a PR is not reviewed within 72 hours, please ping
> the Slack channel so someone can review it.
> 
> What do you think?
> 
> Piotr

Re: Use RTC in Log4j and Log4Net

[Christian Grobmeier]

Hello,

After working on Log4j with PRs, I have changed my opinion on CTR/RTC in this 
case. Previously, I would have said keep CTR. However, I worked with RTC using 
PRs, and my experiences were not bad. I was a bit lost with the comments on the 
PR, but I figured it out somehow. I think GitHub is not really fantastic when 
it comes to commenting and reviewing, but it is better than searching for 
related commits in the commit history.

This alone would not justify CTR, but I think we have a mature project with a 
huge user base and also a history of an issue that caused the internet to stop. 
Considering how critical Log4j might be, I think it warrants asking for reviews 
before we actually change something.

I don't know if all repositories mentioned need this, but log4j 2.x is worth 
being more careful.
Kotlin and Scale - probably, just because we use it on Log4j.
Transform - no idea
Tools - if it's just tooling, maybe it's not worth it
.NET - the .NET devs should decide on that

Generally speaking, the ones who do most of the work (aka reviews) should also 
have a heavyweight in their votes. 

I am a bit concerned about low-profile commits like typos. On the other hand, 
if you fix 20 or 30 typos, then it might be worth having them combined in one 
PR.

Let's give it a try and see where it leads us. We can always change it back if 
we see this is no good. Given the two most active people here already use RTC 
we maybe should just follow the lead

+1

Cheers


On Tue, Sep 17, 2024, at 10:30, Piotr P. Karwasz wrote:
> Hi all,
>
> Inspired by the way the Log4Net team works, I would like to propose a
> switch from our CTR policy to RTC:
>
> * every change to the main branches should go through a pull request,
> * pull requests can be merged if they have 1 positive review and no
> requested changes. To allow everyone to look at the PR, let's not
> merge them before at least 24 hours have passed.
> * if a pull request does not receive any review in 72 hours, as a last
> resort we merge them without a review.
>
> I would like to apply this policy to our most active repos:
>
> * l-log4j2
> * l-log4j-kotlin
> * l-log4j-scala
> * l-log4j-transform
> * l-log4j-tools
> * l-log4net
>
> I am NOT proposing this change to the equally active l-log4cxx
> repository, since the Log4cxx team has an established workflow that
> does not use formal reviews.
>
> I would prefer to explicitly add a branch protection rule that
> requires reviews. If a PR is not reviewed within 72 hours, please ping
> the Slack channel so someone can review it.
>
> What do you think?
>
> Piotr

[log4j] Google OSS-Fuzz integration

[Volkan Yazıcı]

OSS-Fuzz  is a Google service that
continuously runs fuzz tests of critical F/OSS projects on a beefy cluster
and reports its findings (bugs, vulnerabilities, etc.) privately to project
maintainers. In #2949 ,
I implemented fuzz tests for Log4j 2 and their integration with OSS-Fuzz. I
have documented the details in `FUZZING.adoc`
, e.g.,

   - How can I run fuzz tests locally?
   - How can I view fuzzing failures detected by OSS-Fuzz?
   - How can I reproduce fuzzing failures detected by OSS-Fuzz?

If you have any further questions, please let me know. If requested, I can
also provide a walkthrough in the next PMC meeting.

Re: Use RTC in Log4j and Log4Net

[Piotr P. Karwasz]

Hi Matt,

On Tue, 17 Sept 2024 at 20:06, Matt Sicker  wrote:
>
> I’m -1 on switching to RTC. Same reason as always. Losing momentum from 
> waiting for an unnecessary code review will simply lead to much longer gaps 
> between time I spend on the project.

Honestly, momentum is not always a good thing and external input can
be beneficial.
While the content of your contributions was always excellent, the way
you submit your PRs have vastly improved since I know you. It is now
much easier for me to review them.

Piotr

Re: Use RTC in Log4j and Log4Net

[Jan Friedrich]

Hi

we have only had good experiences with RTC in log4net.
Of course, a lot depends on a review being done in a timely manner.
The feedback is always valuable and I can apply the changes before merging to 
main.
But most of the time, a pending review does not stop me from continuing my work.
Either I append my changes to the existing branch (when  related) or I create a 
new branch,
based on the existing one and create the pull request after the first one was 
merged.

So it's a +1 from me.

Regards.

Jan

Re: Use RTC in Log4j and Log4Net

[Gary Gregory]

I'm with Matt. It should be left at the discretion of the developer whether
a PR or straight commit is justified. I see no reason to throw sand in the
gears.

Gary

On Tue, Sep 17, 2024, 2:05 PM Matt Sicker  wrote:

> I’m -1 on switching to RTC. Same reason as always. Losing momentum from
> waiting for an unnecessary code review will simply lead to much longer gaps
> between time I spend on the project.
>
> > On Sep 17, 2024, at 03:30, Piotr P. Karwasz 
> wrote:
> >
> > Hi all,
> >
> > Inspired by the way the Log4Net team works, I would like to propose a
> > switch from our CTR policy to RTC:
> >
> > * every change to the main branches should go through a pull request,
> > * pull requests can be merged if they have 1 positive review and no
> > requested changes. To allow everyone to look at the PR, let's not
> > merge them before at least 24 hours have passed.
> > * if a pull request does not receive any review in 72 hours, as a last
> > resort we merge them without a review.
> >
> > I would like to apply this policy to our most active repos:
> >
> > * l-log4j2
> > * l-log4j-kotlin
> > * l-log4j-scala
> > * l-log4j-transform
> > * l-log4j-tools
> > * l-log4net
> >
> > I am NOT proposing this change to the equally active l-log4cxx
> > repository, since the Log4cxx team has an established workflow that
> > does not use formal reviews.
> >
> > I would prefer to explicitly add a branch protection rule that
> > requires reviews. If a PR is not reviewed within 72 hours, please ping
> > the Slack channel so someone can review it.
> >
> > What do you think?
> >
> > Piotr
>
>

Re: Use RTC in Log4j and Log4Net

[Ralph Goers]

This isn’t a vote so I am not going to. If I had to vote I wouldn’t vote for a 
policy that requires RTC always. However, I would vote for a policy that 
requires RTC when specified criteria are met.

Ralph

> On Sep 17, 2024, at 10:28 AM, Ralph Goers  wrote:
> 
> First, the obvious. I haven’t committed much in a while. The last several I 
> did I used PRs primarily because it makes it easier for people to review the 
> changes but I didn’t necessarily wait for a review.  For really simple stuff 
> I've never use a PR. However, with the switch from Jira to GitHub issues it 
> might make more sense to use a PR since you have to have something to track 
> the problem. But if there is already an issue then I would probably just use 
> that. Again, depending on what is being done.
> 
> I wouldn’t classify any of the work I’ve seen you doing recently as trivial 
> or minor, so you only doing PRs makes sense. 
> 
> Ralph
> 
>> On Sep 17, 2024, at 7:52 AM, Piotr P. Karwasz  
>> wrote:
>> 
>> Hi Ralph,
>> 
>> On Tue, 17 Sept 2024 at 15:47, Ralph Goers  
>> wrote:
>>> 
>>> Why? i.e. - what currently isn’t working?
>> 
>> I merely wish to formalize what is already happening and set up a
>> branch protection rule to enforce it.
>> 
>> Note that I have never seen a PR in Log4Net being merged without a review.
>> 
>> On the other hand you can probably find a couple of my recent PRs that
>> don't have a formal review. Sure, I must have certainly consulted with
>> someone on Slack before I merged them, but there is no sign of it.
>> Let's make it formal, so users also see it.
>> 
>> Piotr
>