elharo commented on code in PR #489:
URL:
https://github.com/apache/maven-build-cache-extension/pull/489#discussion_r3242942940
##########
src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java:
##########
@@ -85,6 +85,7 @@ public class CacheConfigImpl implements
org.apache.maven.buildcache.xml.CacheCon
public static final String CONFIG_PATH_PROPERTY_NAME =
"maven.build.cache.configPath";
public static final String CACHE_ENABLED_PROPERTY_NAME =
"maven.build.cache.enabled";
public static final String CACHE_LOCATION_PROPERTY_NAME =
"maven.build.cache.location";
+ public static final String MAX_LOCAL_BUILDS_CACHED_PROPERTY_NAME =
"maven.build.cache.maxLocalBuildsCached";
Review Comment:
This doesn't appear to need to be public since it's only used in one place
##########
src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java:
##########
@@ -568,7 +569,12 @@ public String getTransport() {
@Override
public int getMaxLocalBuildsCached() {
checkInitializedState();
- return getLocal().getMaxBuildsCached();
+ int maxLocalBuildsCached =
+ getProperty(MAX_LOCAL_BUILDS_CACHED_PROPERTY_NAME,
getLocal().getMaxBuildsCached());
+ if (maxLocalBuildsCached <= 0) {
+ throw new IllegalArgumentException("maxLocalBuildsCached must be
greater than 0");
Review Comment:
This doesn't feel right. This is a system property, not an argument, and it
shouldn't be thrown by a getter method with on arguments. Probably if this can
really happen then this needs to be checked much earlier. And even better just
pick the default value here.
##########
src/site/markdown/parameters.md:
##########
@@ -41,6 +41,7 @@ This document contains various configuration parameters
supported by the cache e
| `-Dmaven.build.cache.skipSave=(true/false)` | Skip writing
build result in caches. Does not affect reading from the cache.
|
Configuring MR builds to benefits from the cache, but restricting writes to the
`master` branch |
| `-Dmaven.build.cache.mandatoryClean=(true/false)` | Enable or
disable the necessity to execute the `clean` phase in order to store the build
result in cache. Default: `false`
|
Reducing the risk to save "wrong" files in cache in a local dev environment
|
| `-Dmaven.build.cache.cacheCompile=(true/false)` | Cache compile
phase outputs (classes, test-classes, generated sources). When enabled
(default), compile-only builds create cache entries that can be restored by
subsequent builds. When disabled, caching only occurs during package phase or
later. | Performance optimization for incremental builds
|
+| `-Dmaven.build.cache.maxLocalBuildsCached=<n>` | Maximum number
of cached build records retained per project in the local cache. Overrides
`<local><maxBuildsCached>` from the XML config. (default: `3`)
|
Limit local cache disk usage or retain more builds for bisecting failures
|
Review Comment:
I'm not sure what the angle brackets on `<local><maxBuildsCached>` mean
here. I guess this is supposed to be XML but pieces are missing. Might be
better without the brackets.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
