#AMQ-8525 Improve performance of mqtt module #1541
Conversation
|
Some stats: activemq-mqtt build times on MBP M3 |
|
Commit message is misleading as it only focuses on improving the test execution time not the MQTT code itself. |
|
So one big problem here is that this is going to couple things to Junit 4. Now that Junit 6 is out we should be looking to migrate towards that otherwise all of this may have to be redone or thrown away so I'm not sure it makes sense to do any of this work until we migrate to an upgraded Junit. |
|
I think we could tackle JUnit 6 migration one module at a time as well. That way we don't need one giant PR (esp for activemq-unit-tests module). |
|
We still have old style JUnit 3 tests. I fully agree we should modernize the tests, a lot need to be rewritten as well and some might be removed all together. Locally, I can observe the same as @mattrpav activemq-mqtt takes
|
|
The issue here is all of the changes are going to make it even harder to migrate which is counter productive when we need to migrate exactly because we are still using Junit 3/4 which is very much out of date. |
|
So I think to follow up the real question is just what happens when we convert tests? is there a path forward or does all of this have to be rewritten again? Some stuff is easy to migrate in Junit 5/6 and other things are completely different |
|
Aside from a few tests where I had to make the data directory dynamic (so parallel executions each get their own directory), the rest of the changes are simply adding @category on test classes that can safely run in parallel. I went through the modules manually to identify which tests were compatible, which took some time. Many of the remaining ones could be migrated as well, but they need deeper refactoring, so they aren’t part of the “quick win” I was aiming for here. The rest is just Surefire configuration. Regarding migrating to JUnit 5/6: I agree that @category → @tag and Surefire → is straightforward. If your suggestion is to go that route instead, I’m perfectly fine with it. Vintage is deprecated but still available in JUnit 6, so it gives us some runway. I also fully agree that most tests ultimately need to be modernized. Many still inherit from TestCase, so we’ll need to add proper @test annotations, use before/after hooks, and switch to assertions for timeouts and exceptions. With the current codebase, that’s a substantial amount of work. I know the community is interested in pushing towards that modernization—which is great—but in the meantime getting faster CI feedback (2–3 hours instead of ~7 hours) would already be a big improvement. What do you think? |
|
If I understand the comment about JUnit 6, I think it's a separate and more global effort. This PR is focusing on test performance improvements on the MQTT module. We need consistency on the contribution. I would rather focus on improvements without other changes, and do the JUnit 6 upgrade effort in another PR. |
There was a problem hiding this comment.
This is a good improvement and perfectly aligned with the initial intent: we keep the tests as they are, improving performance and stability.
That's the scope of this PR, so it's good for me.
Of course, other PRs will follow to do larger refactoring/update. This PR is a good prep step :)
Thanks for pointing out the @tag annotation, I read up a bit on it. I wasn't trying to say we should combine Junit 6 with the performance work, etc. just simply that I didn't want to make it harder to migrate. Many tests are easy to migrate from Junit 4 but some things are much harder (for example only in 5.13 did they finally add class level parameterized tests, etc). I wasn't sure if using the @category tag was going to cause issues with the migration and further delay it, but because we can simply move to @tag I think it's fine to go ahead with this PR. It looks pretty straight forward so it should not be an issue. |
|
I did some more review and I was able to add more tests to parallel execution. I also reworked the output for clarity (see email thread I sent). From 14'23 locally to 3'34 locally |
|
In the CI, went from 20:56 to 5'01 |
|
Are you guys waiting for something else on this one? |
|
Thanks @mattrpav for the review. |
|
@jeanouii checkout my minor update to your branch.
Please rebase and squash your commits to one commit and I think this one is ready to merge. |
ee248f8 to
dca4a39
Compare
dca4a39 to
688236e
Compare
activemq-mqtt/pom.xml
Outdated
| </plugins> | ||
| </build> | ||
| </profile> | ||
| <profile> |
There was a problem hiding this comment.
hmm, looks like a regression in the coverage I'm a bit worried as an user
There was a problem hiding this comment.
Yes the initial purpose was to have a PR that runs the exact same test suite as main, without having to enable or disable a profile.
Introducing a profile duplicates the configuration and can be a worry for test coverage as you pointed out.
The project has already a lot of profiles, excludes in tests, etc. I don't think adding one more for that was necessary.
There was a problem hiding this comment.
hmm, looks like a regression in the coverage I'm a bit worried as an user
Appreciate the review. Would you please expand and provide detail?
There was a problem hiding this comment.
profile is likely to not be ran when contributing/rebuilding so it is always a bad practise when setting up a test suite. Profiles are really for what can't run by default because it needs something special (latest nvidia gpu for ex to be trendy). There, there is no reason since it do run, it just needs another execution if you think it is worth splitting - my 2 cts would be I would make it working in the default execution to ensure a smooth integration in the IDE for contributors but this is more a habit/preference.
activemq-mqtt/pom.xml
Outdated
| <build> | ||
| <plugins> | ||
| <plugin> | ||
| <artifactId>maven-surefire-plugin</artifactId> |
There was a problem hiding this comment.
side note: best practise would be to also set the groupId to qualify the plugin, here you do use a legacy old maven style feature which can lead to conflicts randomly depending user configuration.
There was a problem hiding this comment.
I'd agree and had it in the first place to be honest.
I always put the groupId for consistency and avoid having sometimes implicit versus explicit values.
Maven 4 or 4.1 now prints a warning when there is a duplicate plugin defined. I'm not sure it would work if you have one using implicit and another one using explicit.
|
@mattrpav really I don't see the benefit of moving the tests running in parallel into another profile. |
|
I think one thing about a profile is that it makes it easy to disable (it could be enabled by default). When I run gradle builds (like with Apache Kafka) I sometimes reduce the number of cores/threads used so I don't use up all the CPU if I'm trying to run a build in the background while doing other things. But in this case instead of a separate profile, how many threads are use could probably just be controlled by a -D argument for the surefire plugin vs a separate profile. |
|
@cshannon Agreed. Adding a property to override the default number of threads/cores used is totally ok. |
|
If it helps surefire supports perCoreThreadCount so it can be relative to the machine (but today it needs to be an int) |
|
I used 2C by default as of now, but we can fine tune that as we go or make it configurable |
4ae958f to
ab803ca
Compare
ab803ca to
ffed175
Compare
…read.sleep and improve message assertion logic
|
Can we get a new round on this PR ? As it's about MQTT test and the comments have been addressed, I think we can rebase and move forward. Thoughts ? |
No description provided.