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
Add property to disable model caching #797
Conversation
8771ae4
to
50d75b2
Compare
Hi @ppalaga, would that be something that you can add for us? |
@gnodet didn't we have a property for disabling caching for specific artifacts? |
@ppalaga you have a property for reloading certain class realms (e.g. for plugins that hold static state). But nothing for the model cache. |
Thanks for explaining @oehme! That's exactly, what my foggy recollection was about. |
@@ -45,6 +45,9 @@ public SnapshotModelCacheFactory(DefaultModelCacheFactory factory) { | |||
|
|||
@Override | |||
public ModelCache createCache(RepositorySystemSession session) { | |||
return new SnapshotModelCache(globalCache, factory.createCache(session)); | |||
boolean enableModelCache = Boolean.parseBoolean(System.getProperty("mvnd.modelCache", "true")); |
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.
Could you please add the new property to org.mvndaemon.mvnd.common.Environment
and add a bit of JavaDoc describing what it is doing?
Naming: Looking at other boolean properties, we have in org.mvndaemon.mvnd.common.Environment
, perhaps the mvnd.noModelCache
style would suit best here. I am not especially happy about negating names, but we already have a couple of those, and they are easier to use (given the default is with model cache enabled) from command line: -Dmvnd.noModelCache
vs. -Dmvnd.modelCache=false
. It is less typing. What do others think?
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.
Sure, I'll do that
46cb739
to
827ee0e
Compare
* If <code>true</code>, the daemon will not use its in-memory metadata cache and instead re-read the | ||
* metadata from the pom.xml files in the local repository. This is mostly useful for testing purposes. | ||
*/ | ||
MVND_NO_MODEL_CACHE("mvnd.noModelCache", null, Boolean.FALSE, OptionType.BOOLEAN, Flags.NONE), |
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.
I wonder whether we should use Flags.DISCRIMINATING
? Is the createCache()
method called once for a given daemon instance or once per Maven build?
If it's once for daemon, I'd say it should be Flags.DISCRIMINATING
. Otherwise it could happen that the user starts the daemon with mvnd -Dmvnd.noModelCache
and all subsequent mvnd
invocations with or without -Dmvnd.noModelCache
will be served by the same daemon instance that has model cache off. That would not be correct, would it?
Flags.DISCRIMINATING
will make all calls with -Dmvnd.noModelCache
be served by a cacheless instance while all calls without -Dmvnd.noModelCache
will be served by some other daemon instance that has the cache enabled.
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.
The createCache
method is called every build, so it doesn't have to be discriminating. It doesn't hurt either, I don't mind :)
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.
If it is called every build, then no need to change anything. Thanks!
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.
Looking at the CI failures, we may need Flags.OPTIONAL
.
827ee0e
to
dd5a2e7
Compare
This is mostly for the integration tests of the Gradle Enterprise Maven extension, which heavily test dependency resolution and reuse the same GAVs in many test, e.g. a:b:1.0. It would be a lot of effort to rewrite all those tests and a shame to restart the daemon every time. So I wanted to ask if you'd consider this flag to disable the caching altogether.
dd5a2e7
to
a1a9b53
Compare
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.
Perfect, thanks @oehme!
In case we wanted mvnd.noModelCache
to get honored also in ~/.m2/mvnd.properties
one more tweak would be needed. But I guess it is not really necessary for this one.
Follow-up to apache#797, which didn't get backported to the mvn39 part of the code.
Follow-up to apache#797, which didn't get backported to the mvn39 part of the code.
Follow-up to #797, which didn't get backported to the mvn39 part of the code.
This is mostly for the integration tests of the Gradle Enterprise Maven extension, which heavily test dependency resolution and reuse the same GAVs in many test, e.g.
a:b:1.0
. It would be a lot of effort to rewrite all those tests and a shame to restart the daemon every time. So I wanted to ask if you'd consider this flag to disable the caching altogether.