Skip to content

[MBUILDCACHE-46] Add maven.build.cache.remote.enabled parameter #43

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 30, 2023
Merged

[MBUILDCACHE-46] Add maven.build.cache.remote.enabled parameter #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d843f4d
[MBUILDCACHE-46] Add maven.build.cache.remote.enabled parameter
asteffen-partek Feb 14, 2023
f95ff34
review changes
asteffen-partek Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ under the License.
<commonsCliVersion>1.4</commonsCliVersion>
<commonsLangVersion>3.12.0</commonsLangVersion>
<junitVersion>5.8.1</junitVersion>
<mockitoVersion>3.2.0</mockitoVersion>
<mockitoVersion>4.8.0</mockitoVersion>
<plexusUtilsVersion>3.4.1</plexusUtilsVersion>
<wagonVersion>3.4.3</wagonVersion>
<securityDispatcherVersion>2.0</securityDispatcherVersion>
Expand Down Expand Up @@ -225,6 +225,12 @@ under the License.
<version>${junitVersion}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-junit-jupiter</artifactId>
<version>${mockitoVersion}</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
11 changes: 7 additions & 4 deletions src/main/java/org/apache/maven/buildcache/CacheControllerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,14 @@ public CacheResult findCachedBuild(
LOGGER.info("Attempting to restore project {} from build cache", projectName);

// remote build first
result = findCachedBuild(mojoExecutions, context);
if (cacheConfig.isRemoteCacheEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write integration tests for this flag. In pr #34, I added Wiremock-based integration tests to verify the remote cache behavior. The same approach could be reused here. It will be good to see a test that checks that remote is disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. For testing flag and xml setting combinations, a lightweight unit test seems prudent. I went ahead and added a unit test for CacheConfigImpl. I tested the initialization and the remote parameters only, but structured it in a way that it should easily accommodate tests around other configurations. A couple integration tests make sense as well, but I thought i'd wait until PR [MBUILDCACHE-25] Fixes N+1 cache processings for forked executions #34 is merged to build off that approach. Tests are a great way to guide meaningful cleanups and refactors, and I feel I need some more time and context to do this at an integration level.

  2. Remote configurations are now preconditioned on any meaningful encompassing configuration behind the CacheConfig interface.

Copy link
Contributor

@AlexanderAshitkin AlexanderAshitkin Mar 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarifications, I clicked it!

result = findCachedBuild(mojoExecutions, context);
if (!result.isSuccess() && result.getContext() != null) {
LOGGER.info("Remote cache is incomplete or missing, trying local build for {}", projectName);
}
}

if (!result.isSuccess() && result.getContext() != null) {
LOGGER.info("Remote cache is incomplete or missing, trying local build for {}", projectName);

CacheResult localBuild = findLocalBuild(mojoExecutions, context);
if (localBuild.isSuccess() || (localBuild.isPartialSuccess() && !result.isPartialSuccess())) {
result = localBuild;
Expand Down Expand Up @@ -444,7 +447,7 @@ public void save(
completedExecution,
hashFactory.getAlgorithm());
populateGitInfo(build, session);
build.getDto().set_final(cacheConfig.isSaveFinal());
build.getDto().set_final(cacheConfig.isSaveToRemoteFinal());
cacheResults.put(getVersionlessProjectKey(project), rebuilded(cacheResult, build));

// if package phase presence means new artifacts were packaged
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/apache/maven/buildcache/LocalCacheRepositoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ public void beforeSave(CacheContext environment) {
public void saveBuildInfo(CacheResult cacheResult, Build build) throws IOException {
final Path path = localBuildPath(cacheResult.getContext(), BUILDINFO_XML, true);
Files.write(path, xmlService.toBytes(build.getDto()), TRUNCATE_EXISTING, CREATE);
if (cacheConfig.isRemoteCacheEnabled() && cacheConfig.isSaveToRemote() && !cacheResult.isFinal()) {
if (cacheConfig.isSaveToRemote() && !cacheResult.isFinal()) {
remoteRepository.saveBuildInfo(cacheResult, build);
}
}
Expand All @@ -343,7 +343,7 @@ public void saveCacheReport(String buildId, MavenSession session, CacheReport ca
xmlService.toBytes(cacheReport),
TRUNCATE_EXISTING,
CREATE);
if (cacheConfig.isRemoteCacheEnabled() && cacheConfig.isSaveToRemote()) {
if (cacheConfig.isSaveToRemote()) {
LOGGER.info("Saving cache report on build completion");
remoteRepository.saveCacheReport(buildId, session, cacheReport);
}
Expand All @@ -356,7 +356,7 @@ public void saveArtifactFile(CacheResult cacheResult, org.apache.maven.artifact.
File artifactFile = artifact.getFile();
Path cachePath = localBuildPath(cacheResult.getContext(), CacheUtils.normalizedName(artifact), true);
Files.copy(artifactFile.toPath(), cachePath, StandardCopyOption.REPLACE_EXISTING);
if (cacheConfig.isRemoteCacheEnabled() && cacheConfig.isSaveToRemote() && !cacheResult.isFinal()) {
if (cacheConfig.isSaveToRemote() && !cacheResult.isFinal()) {
remoteRepository.saveArtifactFile(cacheResult, artifact);
}
}
Expand Down
1 change: 0 additions & 1 deletion src/main/java/org/apache/maven/buildcache/RemoteCacheRepositoryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,6 @@ public Optional<Build> findBuild(CacheContext context) throws IOException {
.map(content -> new Build(xmlService.loadBuild(content), CacheSource.REMOTE));
}

@Nonnull
@Override
public boolean getArtifactContent(CacheContext context, Artifact artifact, Path target) throws IOException {
return getResourceContent(getResourceUrl(context, artifact.getFileName()), target);
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/apache/maven/buildcache/xml/CacheConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
import org.apache.maven.plugin.MojoExecution;

/**
* CacheConfig
* A java interface to the information configured in the maven-build-cache-config.xml file
*/
public interface CacheConfig {

Expand Down Expand Up @@ -93,7 +93,7 @@ public interface CacheConfig {

boolean isSaveToRemote();

boolean isSaveFinal();
boolean isSaveToRemoteFinal();

boolean isSkipCache();

Expand Down
45 changes: 27 additions & 18 deletions src/main/java/org/apache/maven/buildcache/xml/CacheConfigImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,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 REMOTE_ENABLED_PROPERTY_NAME = "maven.build.cache.remote.enabled";
public static final String SAVE_TO_REMOTE_PROPERTY_NAME = "maven.build.cache.remote.save.enabled";
public static final String SAVE_NON_OVERRIDEABLE_NAME = "maven.build.cache.remote.save.final";
public static final String FAIL_FAST_PROPERTY_NAME = "maven.build.cache.failFast";
Expand Down Expand Up @@ -124,13 +125,13 @@ public CacheState initialize() {
if (state == null) {
synchronized (this) {
if (state == null) {
final String enabled = getProperty(CACHE_ENABLED_PROPERTY_NAME, "true");
final boolean enabled = getProperty(CACHE_ENABLED_PROPERTY_NAME, true);

if (!rtInfo.isMavenVersion("[3.9.0,)")) {
LOGGER.warn("Cache requires Maven >= 3.9, but version is " + rtInfo.getMavenVersion()
+ ". Disabling cache.");
state = CacheState.DISABLED;
} else if (!Boolean.parseBoolean(enabled)) {
} else if (!enabled) {
LOGGER.info("Cache disabled by command line flag, project will be built fully and not cached");
state = CacheState.DISABLED;
} else {
Expand Down Expand Up @@ -163,12 +164,10 @@ public CacheState initialize() {

// `maven.build.cache.enabled` overrides the `enabled` of the XML file
// to allow a disabled configuration to be enabled on the command line
final String cacheEnabledProperty = getProperty(CACHE_ENABLED_PROPERTY_NAME, null);
if (cacheEnabledProperty != null) {
cacheConfig.getConfiguration().setEnabled(Boolean.parseBoolean(cacheEnabledProperty));
}
boolean cacheEnabled = getProperty(
CACHE_ENABLED_PROPERTY_NAME, getConfiguration().isEnabled());

if (!cacheConfig.getConfiguration().isEnabled()) {
if (!cacheEnabled) {
state = CacheState.DISABLED;
} else {
String hashAlgorithm = null;
Expand Down Expand Up @@ -452,28 +451,29 @@ public boolean isEnabled() {
@Override
public boolean isRemoteCacheEnabled() {
checkInitializedState();
return getRemote().getUrl() != null && getRemote().isEnabled();
return getRemote().getUrl() != null
&& getProperty(REMOTE_ENABLED_PROPERTY_NAME, getRemote().isEnabled());
}

@Override
public boolean isSaveToRemote() {
checkInitializedState();
return Boolean.getBoolean(SAVE_TO_REMOTE_PROPERTY_NAME) || getRemote().isSaveToRemote();
return isRemoteCacheEnabled()
&& getProperty(SAVE_TO_REMOTE_PROPERTY_NAME, getRemote().isSaveToRemote());
}

@Override
public boolean isSaveFinal() {
return Boolean.getBoolean(SAVE_NON_OVERRIDEABLE_NAME);
public boolean isSaveToRemoteFinal() {
return isSaveToRemote() && getProperty(SAVE_NON_OVERRIDEABLE_NAME, false);
}

@Override
public boolean isSkipCache() {
return Boolean.getBoolean(CACHE_SKIP);
return getProperty(CACHE_SKIP, false);
}

@Override
public boolean isFailFast() {
return Boolean.getBoolean(FAIL_FAST_PROPERTY_NAME);
return getProperty(FAIL_FAST_PROPERTY_NAME, false);
}

@Override
Expand All @@ -488,14 +488,12 @@ public String getBaselineCacheUrl() {

@Override
public boolean isLazyRestore() {
final String lazyRestore = getProperty(LAZY_RESTORE_PROPERTY_NAME, "false");
return Boolean.parseBoolean(lazyRestore);
return getProperty(LAZY_RESTORE_PROPERTY_NAME, false);
}

@Override
public boolean isRestoreGeneratedSources() {
final String restoreGeneratedSources = getProperty(RESTORE_GENERATED_SOURCES_PROPERTY_NAME, "true");
return Boolean.parseBoolean(restoreGeneratedSources);
return getProperty(RESTORE_GENERATED_SOURCES_PROPERTY_NAME, true);
}

@Override
Expand Down Expand Up @@ -597,4 +595,15 @@ private String getProperty(String key, String defaultValue) {
}
return value;
}

private boolean getProperty(String key, boolean defaultValue) {
String value = session.getUserProperties().getProperty(key);
if (value == null) {
value = session.getSystemProperties().getProperty(key);
if (value == null) {
return defaultValue;
}
}
return Boolean.parseBoolean(value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think with system properties this is redundant, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be accomplished any number of ways, but I decided to go with the micro-optimized form that does not require a string parse to handle the default case.

}
}
1 change: 1 addition & 0 deletions src/site/markdown/parameters.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ This documents contains various configuration parameters supported by cache engi
|------------------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------------------|
| `-Dmaven.build.cache.configPath=path to file` | Location of cache configuration file | Cache config is not in default location |
| `-Dmaven.build.cache.enabled=(true/false)` | Cache and associated features disabled/enabled | To remove noise from logs then remote cache is not available |
| `-Dmaven.build.cache.remote.enabled=(true/false)` | Checks and downloads artifacts from the remote cache (overrides <remote enabled=("true"/"false")>) | To control remote cache access by node, if, say, some nodes lack reliable access |
| `-Dmaven.build.cache.remote.save.enabled=(true/false)` | Remote cache save allowed or not | To designate nodes which allowed to push in remote shared cache |
| `-Dmaven.build.cache.remote.save.final=(true/false)` | Prohibit to override remote cache | To ensure that reference build is not overridden by interim build |
| `-Dmaven.build.cache.failFast=(true/false)` | Fail on the first module which cannot be restored from cache | Remote cache setup/tuning/troubleshooting |
Expand Down
Loading