-
Notifications
You must be signed in to change notification settings - Fork 1.5k
#AMQ-8525 Improve performance of mqtt module #1541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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). |
…ne arguments are used
|
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. |
jbonofre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
No description provided.