Skip to content

Conversation

jeantil
Copy link
Contributor

@jeantil jeantil commented Feb 18, 2024

Context

Enabling injectAllReactorProjects in large and old reactor projects allows to avoid repeatedly parsing the git history to retrieve the information.

In the apache/james-project enabling this property allowed to lower the time spent in git-commit-id from 39s[1] to 2s[2].

However, looking at the verbose logs one can see that the plugin keeps injecting the properties into every reactor project for every project where the plugin runs.

This commit aims to avoid reinjecting keys which have already been set in the context.

[1] https://ge.apache.org/s/xumcl7ztpkmhw/timeline?collapse-all&outcome=success,failed&view=by-type
[2] https://ge.apache.org/s/gpnevfknmdv5a/timeline?collapse-all&hide-timeline&outcome=success,failed&view=by-type

Contributor Checklist

  • Added relevant integration or unit tests to verify the changes
  • Update the Readme or any other documentation (including relevant Javadoc)
  • Ensured that tests pass locally: mvn clean package
  • Ensured that the code meets the current checkstyle coding style definition: mvn clean verify -Pcheckstyle -Dmaven.test.skip=true -B

I'm not saying the fix is perfect, but hopefully it illustrates the issue. If you want a sample project to reproduce you can look at https://github.com/apache/james-project/ enabling verbose output on git-commit-id you will be able to see that for every single project the plugin reinjects all the properties in the 273 projects of the reactor ... effectively setting the properties 273*273 =74529 times.
The performance impact is not huge since its all in memory accesses but it still feels quite wasteful

Enabling `injectAllReactorProjects` in large and old reactor projects
allows to avoid repeatedly parsing the git history to retrieve the
information.

In the apache/james-project enabling this property allowed to lower the
time spent in git-commit-id from 39s[1] to 2s[2].

However, looking at the verbose logs one can see that the plugin keeps
injecting the properties into every reactor project for every project
where the plugin runs.

This commit aims to avoid reinjecting keys which have already been set
in the context.

[1] https://ge.apache.org/s/xumcl7ztpkmhw/timeline?collapse-all&outcome=success,failed&view=by-type
[2] https://ge.apache.org/s/gpnevfknmdv5a/timeline?collapse-all&hide-timeline&outcome=success,failed&view=by-type
@TheSnoozer
Copy link
Collaborator

Thanks for all the details and your contribution to this project :-)
Indeed the injectAllReactorProjects would make the properties available to all projects and each project would try to extract the information from the injected context (if available).

I can't see anything wrong in your contribution and your changes make a lot of sense to me. Let's try to avoid wasteful things :-)

Perhaps worth to mention that you can also tell the plugin to execute only once by using the <runOnlyOnce>true</runOnlyOnce> option. With this option enabled the plugin will get executed, but decides to skip itself if currentProject is not the first in the execution chain....but then I doubt it would make anything much faster in this case.

mais maintenant merci encore pour votre contribution :-)

@TheSnoozer TheSnoozer added this to the 7.0.1 milestone Feb 18, 2024
@TheSnoozer TheSnoozer merged commit d86f923 into git-commit-id:master Feb 18, 2024
@jeantil
Copy link
Contributor Author

jeantil commented Feb 18, 2024

Wow I never expected this to be reviewed and merge that fast, thanks !

Perhaps worth to mention that you can also tell the plugin to execute only once by using the true option.

I did notice about this option, unfortunately it can' t be used for the james project. Most of the projects in the the reactor are made available on apache snapshot repository and released on maven central. To ease diagnostic when a problem occurs we embed a small json file into every jars.
Using runOnlyOnce not only resolves the git properties once but also only generates the properties file for the first jar of the reactor. It so happens that the first jar of the reactor is a test utility jar for which the git properties are not as useful as for other jars since they are less likely to have been pulled by james users.
I tried various schemes to generate the file only once and copy it everywhere but maven makes such tasks quite difficult.

In the end injectAllReactorProjects and the properties caching is a good way to solve this :D

cheers !

@LinWanCen
Copy link

Can we also merge this feature in version 4. x.x, as many projects use JDK 1.8.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants