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






Reply via email to