Ok I will cancel the current vote to include this change and tests. I will test and review the PR.
Thanks ! Regards JB > Le 4 févr. 2022 à 15:33, Colm O hEigeartaigh <cohei...@apache.org> a écrit : > > I submitted an initial PR here: https://github.com/apache/activemq/pull/754 > > Feel free to change it or incorporate it into something else as you > see fit. It would be nice to get it into 5.16.4 if possible... > > Colm. > >> On Fri, Feb 4, 2022 at 1:39 PM JB Onofré <j...@nanthrax.net> wrote: >> >> It sounds good to me. >> >> Thanks for the proposal. I can work on it. >> >> Regards >> JB >> >>>> Le 4 févr. 2022 à 12:17, Colm O hEigeartaigh <cohei...@apache.org> a écrit >>>> : >>> >>> Hey JB/all, >>> >>> I'm wondering if we can also take the opportunity to consider >>> switching from Apache Log4J 1.x to Reload4J for AMQ 5.15.x/5.16.x. >>> https://reload4j.qos.ch/ is a fork of Log4J 1.x with the CVE fixes >>> applied. Other Apache projects such as Apache Avro have switched over >>> to use it, similarly pax logging >>> (https://github.com/ops4j/org.ops4j.pax.logging/commit/a21b1b41fb107164522e587643e8daff2724ce86). >>> >>> There will be many users of AMQ who will not want or be able to >>> migrate to AMQ 5.17 and hence Log4J 2. It's difficult to convey to >>> users that the CVEs don't apply to the default configuration, as many >>> users just see critical CVEs via their own scanning tools and insist >>> on upgrades. In addition, Log4J 1.x is not maintained since 2015 and >>> if a new CVE were to come out that affects the default configuration, >>> it would be problematic. >>> >>> If you approve I can create a PR. >>> >>> Colm. >>> >>>> On Fri, Feb 4, 2022 at 7:51 AM Jean-Baptiste Onofré <j...@nanthrax.net> >>>> wrote: >>>> >>>> Hi Clebert, >>>> >>>> Good idea ! >>>> >>>> If Chris agree with this plan, I'm fine with it. >>>> >>>> Let me work on the tests then (I will create PRs). >>>> >>>> Regards >>>> JB >>>> >>>>> On 04/02/2022 00:55, Clebert Suconic wrote: >>>>> I would say.. write the tests... if they pass no need to cancel the >>>>> release as they will not affect runtime. only cancel the release if >>>>> you actually find an issue. >>>>> >>>>> On Thu, Feb 3, 2022 at 8:15 AM Jean-Baptiste Onofré <j...@nanthrax.net> >>>>> wrote: >>>>>> >>>>>> Hi Chris, >>>>>> >>>>>> OK, fair enough for me. I will cancel the vote and work with Matt to add >>>>>> the tests. >>>>>> >>>>>> Thanks, >>>>>> Regards >>>>>> JB >>>>>> >>>>>> On 03/02/2022 13:57, Christopher Shannon wrote: >>>>>>> JB, >>>>>>> >>>>>>> I would rather just add tests first and re-run the vote as it sounded >>>>>>> like >>>>>>> it wouldn't take too long to add a few tests. I wanted to verify some of >>>>>>> those fixes work and make sense and it would be a lot easier not having >>>>>>> to >>>>>>> try and figure out how to set up a manual test that will most likely be >>>>>>> inconsistent from what others used depending on the config settings and >>>>>>> testing scenarios. Furthermore, it's very common to think an issue is >>>>>>> fixed >>>>>>> from manual testing and then run through unit tests and then find an >>>>>>> issue >>>>>>> (missing edge case, etc) >>>>>>> >>>>>>> Matt, for AMQ-8397 you are right that it would be tricky to test the >>>>>>> behavior itself but I don't think that's necessary. Since the fix is >>>>>>> really >>>>>>> just about adding a new flag to disable sending to the DLQ I would say >>>>>>> instead of testing the full behavior you could at least just write a >>>>>>> quick >>>>>>> test that would verify the default is what you want (off by default) and >>>>>>> that changing the policy actually properly sets it on the destination as >>>>>>> intended. >>>>>>> >>>>>>> On Thu, Feb 3, 2022 at 3:44 AM Jean-Baptiste Onofré <j...@nanthrax.net> >>>>>>> wrote: >>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> As some features have been tested manually (you did a pass, I did a >>>>>>>> quick pass on some as well), it's good enough for the release. >>>>>>>> Agree with Chris that we should add test to "secure" the code >>>>>>>> coverage/regression, but it should be done. >>>>>>>> Either way is fine for me: if we consider it's blocker, I can cancel >>>>>>>> the >>>>>>>> current vote, include new tests and cut a new release. >>>>>>>> Else, I would say, let's move forward with the current vote and include >>>>>>>> tests asap (decoupled from this vote/release). >>>>>>>> >>>>>>>> Regards >>>>>>>> JB >>>>>>>> >>>>>>>> On 02/02/2022 22:21, Matt Pavlovich wrote: >>>>>>>>> Note— I did test these changes manually. >>>>>>>>> >>>>>>>>>> On Feb 2, 2022, at 2:55 PM, Matt Pavlovich <mattr...@gmail.com> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> Hey Christopher- >>>>>>>>>> >>>>>>>>>> I see your point on the wording of AMQ-8443. It is an edge case with >>>>>>>> ConnectionControl, not failover in general. I’ll clear up the >>>>>>>> description. >>>>>>>> I’m happy to add tests for a couple of them; however, AMQ-8397 is >>>>>>>> tricky. >>>>>>>>>> >>>>>>>>>> What do you suggest for a reliable test scenario for AMQ-8397? >>>>>>>>>> >>>>>>>>>> AMQ-8053 - This is a very minor change, I’ll add a test >>>>>>>>>> AMQ-8443 - Edge case, I’ll add a test >>>>>>>>>> AMQ-8397 - Note— this is ‘disabled by default in 5.16.x’. This is a >>>>>>>> reporting change on an edge case from being “move message to DLQ” to >>>>>>>> log >>>>>>>> message and increment a counter. This is really difficult to >>>>>>>> consistently >>>>>>>> create a unit test to produce the scenario on varying hardware. This >>>>>>>> is a >>>>>>>> heavy contention scenario and I was not able to find any tests that >>>>>>>> produce >>>>>>>> the current behavior. I’d be happy to adapt verification if you can >>>>>>>> provide >>>>>>>> a stable base scenario (or point to an existing unit test that I may >>>>>>>> have >>>>>>>> missed). >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Matt Pavlovich >>>>>>>>>> >>>>>>>>>>> On Feb 2, 2022, at 12:56 PM, Christopher Shannon < >>>>>>>> christopher.l.shan...@gmail.com >>>>>>>> <mailto:christopher.l.shan...@gmail.com>> >>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> Unfortunately I am -1. I just checked the release and several Jiras >>>>>>>> don't >>>>>>>>>>> have any unit tests for verification. >>>>>>>>>>> >>>>>>>>>>> https://issues.apache.org/jira/browse/AMQ-8053 < >>>>>>>> https://issues.apache.org/jira/browse/AMQ-8053> >>>>>>>>>>> https://issues.apache.org/jira/browse/AMQ-8372 >>>>>>>>>>> https://issues.apache.org/jira/browse/AMQ-8397 >>>>>>>>>>> https://issues.apache.org/jira/browse/AMQ-8443 >>>>>>>>>>> >>>>>>>>>>> There might be more, I quit checking after the first several. All of >>>>>>>> these >>>>>>>>>>> fixes and improvements need tests. It's important to add some tests >>>>>>>>>>> to >>>>>>>>>>> verify the actual bug or issue and to make sure it doesn't get >>>>>>>>>>> broken >>>>>>>> in >>>>>>>>>>> the future. I am not comfortable releasing this without having tests >>>>>>>> for >>>>>>>>>>> these issues. The same of course goes into anything with 5.17.0. >>>>>>>> Obviously >>>>>>>>>>> we don't need tests for trivial stuff (dependency upgrades, null >>>>>>>> pointer >>>>>>>>>>> checks, etc) but other stuff should be tested. >>>>>>>>>>> >>>>>>>>>>> The biggest issue I have is AMQ-8443 which claims this is a fix for >>>>>>>>>>> the >>>>>>>>>>> "failover transport reconnect not working" but no demonstration of >>>>>>>>>>> the >>>>>>>>>>> issue or how the fix works in a unit test. >>>>>>>>>>> >>>>>>>>>>> On Wed, Feb 2, 2022 at 3:29 AM Francois Papon < >>>>>>>> francois.pa...@openobject.fr> >>>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>>> +1 (non-binding) >>>>>>>>>>>> >>>>>>>>>>>> Thanks! >>>>>>>>>>>> >>>>>>>>>>>> regards, >>>>>>>>>>>> >>>>>>>>>>>> François >>>>>>>>>>>> >>>>>>>>>>>> On 01/02/2022 21:41, Jean-Baptiste Onofre wrote: >>>>>>>>>>>>> Hi everyone, >>>>>>>>>>>>> >>>>>>>>>>>>> I submit Apache ActiveMQ 5.16.4 release to your vote. >>>>>>>>>>>>> >>>>>>>>>>>>> This release includes important fixes and updates on the 5.16.x >>>>>>>> series, >>>>>>>>>>>> especially: >>>>>>>>>>>>> - fix couple of warnings/issues on JDK16+ >>>>>>>>>>>>> - fix stack trace display on transports >>>>>>>>>>>>> - better secure on WebConsole >>>>>>>>>>>>> - several dependencies updates >>>>>>>>>>>>> - and much more! >>>>>>>>>>>>> >>>>>>>>>>>>> Please take a look on the Release Notes for details: >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>> https://issues.apache.org/jira/secure/ReleaseNote.jspa?projectId=12311210&version=12350483 >>>>>>>>>>>>> >>>>>>>>>>>>> Maven staging repository is: >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>> https://repository.apache.org/content/repositories/orgapacheactivemq-1244/ >>>>>>>>>>>>> >>>>>>>>>>>>> Dist staging repository is: >>>>>>>>>>>>> https://dist.apache.org/repos/dist/dev/activemq/activemq/5.16.4/ >>>>>>>>>>>>> >>>>>>>>>>>>> Git tag: >>>>>>>>>>>>> activemq-5.16.4 >>>>>>>>>>>>> >>>>>>>>>>>>> Please vote to approve this release: >>>>>>>>>>>>> >>>>>>>>>>>>> [ ] +1 Approve the release >>>>>>>>>>>>> [ ] -1 Don't approve the release (please provide specific >>>>>>>>>>>>> comments) >>>>>>>>>>>>> >>>>>>>>>>>>> This vote will be open for at least 72 hours. >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks ! >>>>>>>>>>>>> Regards >>>>>>>>>>>>> JB >>>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>> >>>>> >>>>> >>
