jms: cleans up artemis code in attempts to deflake test #1394
Conversation
Signed-off-by: Adrian Cole <adrian@tetrate.io>
There was a problem hiding this comment.
here are some notes in case helps. If you don't have time, I can swing the axe again tomorrow.
here are the places I switched to docker due to flakey embedded servers
https://github.com/openzipkin/zipkin/tree/master/zipkin-collector/activemq/src/test/java/zipkin2/collector/activemq
https://github.com/openzipkin/zipkin-reporter-java/tree/master/activemq-client/src/test/java/zipkin2/reporter/activemq
| @@ -40,7 +40,7 @@ | |||
| * {@link brave.jakarta.jms.ITTracingMessageConsumer} | |||
| */ | |||
| class ITTracingJMSConsumer extends ITJms { | |||
| @RegisterExtension ArtemisJmsExtension jms = new ArtemisJmsExtension(); | |||
| @RegisterExtension JmsExtension jms = new JmsExtension(); | |||
There was a problem hiding this comment.
the non-artemis one was dead code in this project
| } catch (JMSException e) { | ||
| throw new AssertionError(e); | ||
| } | ||
| } | ||
|
|
||
| static class ActiveMQ extends JmsExtension { |
There was a problem hiding this comment.
this was dead code so I flattened the type.
| topicConnection.close(); | ||
| queueSession.close(); | ||
| queueConnection.close(); | ||
| if (session != null) session.close(); |
There was a problem hiding this comment.
when the first flake happened, it was obscured by a NPE, so I fixed in both places
|
also, for some reason, in the other PR only the JDK11 run of the jms-jakarta invoker test was flaking, 21 hasn't so far. |
This is very interesting ... shouldn't be JDK dependent ... |
| @@ -20,13 +20,14 @@ | |||
| <modelVersion>4.0.0</modelVersion> | |||
|
|
|||
| <groupId>@project.groupId@</groupId> | |||
| <artifactId>jms40</artifactId> | |||
There was a problem hiding this comment.
Sorry about that, it was definitely my mistake
| import org.apache.activemq.artemis.jms.client.ActiveMQMessage; | ||
| import org.junit.jupiter.api.extension.AfterEachCallback; | ||
| import org.junit.jupiter.api.extension.BeforeEachCallback; | ||
| import org.junit.jupiter.api.extension.ExtensionContext; | ||
|
|
||
| public abstract class JmsExtension implements BeforeEachCallback, AfterEachCallback { | ||
| /** | ||
| * ActiveMQ 6 supports JMS 2.0, but requires JDK 17. We have to build on JDK |
There was a problem hiding this comment.
👍 was also aware of that but JDK-17 is indeed a hard requirement
@codefromthecrypt happy to look into that (it may took me a few days), we have fought a few flaky tests in Apache CXF related to Artemis migration |
There was a problem hiding this comment.
@codefromthecrypt I was running the tests (from this pull request) for at least an hour continuously and no flaky failures observed so far, this is good cleanup in any case to be merged, thanks!
|
@reta agree to merge this. I'm wondering if CI was having a bad day. In any case, we can revisit later as isn't currently a blocker. |
|
opened an issue as the flake returned https://github.com/openzipkin/brave/actions/runs/7438289752/job/20236893811?pr=1395 |
This extracts code I started to refactor while trying to get to the bottom of this error on an unrelated PR.
@reta this is on a shared branch, if you can see anything we can do to avoid flakes in activemq, please give a try. Note the failure was in the jarkarta invoker tests, and possibly resource related in CI. If there's a way to make things leaner maybe it will pass. Also, if there'a another MQ we don't need to stick with artemis either. In zipkin and reporter, I moved to docker for reasons like this. However, since this is instrumenting, I'm not sure if we can or not.. I guess we could as we arent' instrumenting the broker... Anyway, if you have some time to give a try, please do as this flake is locking up the project. If we can use docker, yank the ActiveMQExtension from zipkin or reporter and swap it out!