-
Notifications
You must be signed in to change notification settings - Fork 3k
Speed up Flyway by preventing it from doing classpath scanning #9531
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
Conversation
88b4953 to
937cb4d
Compare
|
It seems like our |
ba96126 to
a5e9bdc
Compare
9b35940 to
e08dd3e
Compare
b2416d3 to
5a1c57a
Compare
|
Windows issue fixed - PR is now ready |
|
hum.. it's a cool approach but it bothers me to set such a precedent. Do we really want to "hot patch" libraries this way? If they are bad let them be bad.. or have them accept a patch? I suppose there might be situations in which projects aren't reactive while we need a critical fix, but I'd rather us consider forking a project in such a case. Did you send a PR? At least give them a chance.. |
|
@Sanne I share your concerns. I definitely wouldn't have done it had there been a better way. Here is the ticket I opened at Flyway, but so far no response. I don't want to submit a PR there until they at least agree that it's something they want. The reasons I went ahead with this PR here are:
|
Have people complain / contribute fixes to Flyway ? Are we going to fix and improve any project we integrate with? |
|
Of course we can't fix everything, but some commonly used things we can, wouldn't you agree? Moreover, this isn't just a simple speed up, it's, it brings the advantages of our powerful build time processing to the integration with a popular library. |
|
I also need to mention that this monkey patch is the same one we already use for the native image. |
|
Sure, good points @geoand . Don't take my comments as a negative review - just don't be surprised if you'll have to fix more bugs this way going forward :-P |
|
I completely understand @Sanne and I definitely appreciate the scrutiny 👌 |
extensions/flyway/deployment/src/main/java/io/quarkus/flyway/ScannerTransformer.java
Show resolved
Hide resolved
This PR essentially takes the substitution we had for native mode and turns it into a class transformer. This way even in JVM mode our Flyway integration take advantage of the fact that the migration script locations are known at build time, thus no classpath scanning is needed. This solution isn't ideal from a maintenance perspective, but the transformation is relatively straightforward and our test suite for Flyway pretty extensive, so we should easily be able to adapt to future Flyway updates (ideally we would be able to remove this if /when flyway/flyway#2822 is done) Relates to: quarkusio#9428
extensions/flyway/deployment/src/main/java/io/quarkus/flyway/ScannerTransformer.java
Show resolved
Hide resolved
|
I'm leaning towards backporting this to 1.5 Final. @geoand WDYT? |
|
It's should give a nice speed boost and given our extensive Flyway testsuite, I think it's safe |
|
Thanks @geoand for pushing this forward as I can see 'flyway' aren't being as responsive |
|
You are welcome |
This PR essentially takes the substitution we had for native mode
and turns it into a class transformer.
This way even in JVM mode our Flyway integration take advantage of
the fact that the migration script locations are known at build time,
thus no classpath scanning is needed.
This solution isn't ideal from a maintenance perspective, but the
transformation is relatively straightforward and our test suite for
Flyway pretty extensive, so we should easily be able to adapt to future
Flyway updates (ideally we would be able to remove this if /when
flyway/flyway#2822 is done)
Relates to: #9428