Skip to content
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

CURATOR-682: Use Gradle Enterprise Maven Extension to normalize Bnd-LastModified #471

Closed
wants to merge 1 commit into from

Conversation

clayburn
Copy link

This change uses the Runtime classpath normalization feature of the Gradle Enterprise Maven Extension in order to better receive cache hits on surefire goals when the only change is to the Bnd-LastModified attribute of jar manifests.

To see the full impact of this change, one can run an experiment from this project's master branch, then from this project's branch. For this experiment, run two clean verify builds. The first build should populate the cache, while the second should read from the cache. On master, the experiment will show a cache miss. With this change, the experiment will show a cache hit.

This change will allow for cache hits on surefire goals despite the constantly changing value of the Bnd-LastModified attribute of the Manifest
@kezhuw kezhuw changed the title Use Gradle Enterprise Maven Extension to normalize Bnd-LastModified CURATOR-682: Use Gradle Enterprise Maven Extension to normalize Bnd-LastModified Aug 7, 2023
@kezhuw kezhuw self-requested a review August 7, 2023 09:28
@kezhuw
Copy link
Member

kezhuw commented Aug 7, 2023

The Gradle Enterprise Maven Extension offers the ability to cache the results of surefire tests. When the timestamp value of "Bnd-LastModified" is changing on every build, the cache is invalidated and a cache hit never occurs for these expensive tests.

On master, the experiment will show a cache miss. With this change, the experiment will show a cache hit.

Might be stupid, but I am lost in verify this. Are we going to "cache the results of surefire tests" ?

I think when we run tests, we are also test for concurrency and randomness or even flaky tests. Basically, I don't think we should do that if it is the intend.

For normal classes, I saw "Loaded from the build cache" in e44115e, which is HEAD^ of this pr, with clean "~/.m2/.gradle-enterprise/build-cache/v1/" directory and "mvn clean package -DskipTests", "mvn clean verify" in order.

@kezhuw
Copy link
Member

kezhuw commented Aug 7, 2023

I verified this with two successive mvn clean verify on HEAD of this pr. The last one completes without run tests. After changes to one test file in curator-recipes, mvn clean verify runs no tests from curator-framework.

I see the value of this pr(incremental test) and I think Gradle is building something great, but it cloud also puzzle people without any concepts of local cache (especially test cache ?). In most people's knowledge, mvn clean verify will run all tests from ground up. It would be really great that mvn clean cloud clean the corresponding test cache in this sense.

I think we can go with either way:

  1. Document the local cache behavior(including this pr) and way to bypass or disable them.
  2. Enable this through explicit configuration. I certainly want to taste it if we finally go this way and the "explicit" could be configurable.

@eolivelli @tisonkun What do you think ?

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Thank for your contribution.

Curator is not that big and it is a OSS project, we should limit the number of third party maven plugins and add them only of strictly needed.
Once we add it it won't we easy to drop it.

I don't see much value on Curator for this patch, especially as I usually use my IDE to run the tests and I see all tests running only on CI.

So I am not supportive of this patch, but I won't block it if there are other people who think it is a good addition to our build toolkit

@tisonkun
Copy link
Member

I second @eolivelli's suggestion that we may not need this "improvement" as YAGNI.

@clayburn
Copy link
Author

Thanks, I will close it then.

@clayburn clayburn closed this Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants