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

[MCLEAN-95] Add a fast clean option #6

Merged
merged 25 commits into from Jan 10, 2022
Merged

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Dec 16, 2021

Jira issue: https://issues.apache.org/jira/browse/MCLEAN-95

This PR provides a fast clean option which can moves the target directory out of the way and actually deletes files in the background. Idea from apache/maven-mvnd#536 ...

For simple configurations (no filesets defined and no follow links), the directory to be deleted will be moved atomically to a different location (${maven.multiModuleProjectDirectory}/target/.clean by default) and a background thread is started to actually perform the deletion. If the atomic move can not be done, the usual deletion process will take place. When the maven session ends, a wait will happen to make sure all files are actually deleted. In case the deletion has not completed, when the deletion thread is started, it will first remove all pending files.

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

look like new feature ... jira issue will be appreciated

@gnodet
Copy link
Contributor Author

gnodet commented Dec 16, 2021

look like new feature ... jira issue will be appreciated

Yes, I will, I was first checking if there would be some reluctance or not.

@michael-o
Copy link
Member

Will recheck tomorrow

@gnodet gnodet changed the title Add a fast clean option [MCLEAN-95] Add a fast clean option Dec 17, 2021
@gnodet
Copy link
Contributor Author

gnodet commented Dec 17, 2021

Btw, I had to use reflection to access the EventSpyDispatcher because the package is not exported. I don't think this is correct, especially as the object is available from the MavenExecutionRequest interface.

@rmannibucau
Copy link

@gnodet normally you can inject spies directly and do not need to unwrap them from the dispatcher, no?

@gnodet
Copy link
Contributor Author

gnodet commented Dec 17, 2021

@gnodet normally you can inject spies directly and do not need to unwrap them from the dispatcher, no?

From a plugin ? My understanding is that EventSpy and AbstractMavenLifecycleParticipant only work for extensions, not for plugins.

@rmannibucau
Copy link

@gnodet the parent IoC is active while classes are visible I think so it should work, or did you mean EventSpy is not exported for plugins (thought it was). You can't call init/close methods since the lifecycle is managed by the dispatcher but calling a "fire" method should be fine. I don't recall without checking if you can inject directly List<SessionListener> with guice but it sounds like the solution if it works.

@gnodet
Copy link
Contributor Author

gnodet commented Dec 17, 2021

@gnodet the parent IoC is active while classes are visible I think so it should work, or did you mean EventSpy is not exported for plugins (thought it was). You can't call init/close methods since the lifecycle is managed by the dispatcher but calling a "fire" method should be fine. I don't recall without checking if you can inject directly List<SessionListener> with guice but it sounds like the solution if it works.

EventSpy is exported, but plugins can not define such beans as they lookup is done in the container's realm which contains extensions, but not plugins afaik. I can double check that with a real test.

The problem here is that I needed a way for the plugin to be notified when the session ends and this is an unusual case because plugins are not really supposed to outlive their execution. Anyway, the way I found is to use mavenSession.getRequest().getEventSpyDispatcher().add(spy) but the EventSpyDispatcher class in in the org.apache.maven.eventspy.internal package which is not exported. This is bad because the method getEventSpyDispatcher() is itself part of the public api. Makes more sense ?

@rmannibucau
Copy link

@gnodet it was more to inject the spies to not use reflection, oh I see what you mean now, I read it the other way, sory about that

@gnodet
Copy link
Contributor Author

gnodet commented Dec 17, 2021

@gnodet the parent IoC is active while classes are visible I think so it should work, or did you mean EventSpy is not exported for plugins (thought it was). You can't call init/close methods since the lifecycle is managed by the dispatcher but calling a "fire" method should be fine. I don't recall without checking if you can inject directly List<SessionListener> with guice but it sounds like the solution if it works.

EventSpy is exported, but plugins can not define such beans as they lookup is done in the container's realm which contains extensions, but not plugins afaik. I can double check that with a real test.

The problem here is that I needed a way for the plugin to be notified when the session ends and this is an unusual case because plugins are not really supposed to outlive their execution. Anyway, the way I found is to use mavenSession.getRequest().getEventSpyDispatcher().add(spy) but the EventSpyDispatcher class in in the org.apache.maven.eventspy.internal package which is not exported. This is bad because the method getEventSpyDispatcher() is itself part of the public api. Makes more sense ?

Actually, this is quite problematic as the getEventSpies() method has been removed from 4.x branch. We definitely need a clean API for that. I've fixed the code to use reflection on the field instead and raised https://issues.apache.org/jira/browse/MNG-7367 to provide a cleaner API.

pom.xml Outdated Show resolved Hide resolved
src/main/java/org/apache/maven/plugins/clean/Cleaner.java Outdated Show resolved Hide resolved
src/main/java/org/apache/maven/plugins/clean/Cleaner.java Outdated Show resolved Hide resolved
src/main/java/org/apache/maven/plugins/clean/Cleaner.java Outdated Show resolved Hide resolved
src/main/java/org/apache/maven/plugins/clean/Cleaner.java Outdated Show resolved Hide resolved
src/main/java/org/apache/maven/plugins/clean/Cleaner.java Outdated Show resolved Hide resolved
src/main/java/org/apache/maven/plugins/clean/Cleaner.java Outdated Show resolved Hide resolved
src/main/java/org/apache/maven/plugins/clean/Cleaner.java Outdated Show resolved Hide resolved
src/main/java/org/apache/maven/plugins/clean/Cleaner.java Outdated Show resolved Hide resolved
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

A few more nits

# Conflicts:
#	src/main/java/org/apache/maven/plugins/clean/Cleaner.java
@gnodet gnodet merged commit 11c8c87 into apache:master Jan 10, 2022
srowen pushed a commit to apache/spark that referenced this pull request Feb 9, 2023
### What changes were proposed in this pull request?
This pr aims upgrade maven plugins including:

- maven-checkstyle-plugin: from 3.2.0 to 3.2.1
- maven-clean-plugin: from 3.1.0 to 3.2.0
- maven-dependency-plugin: from 3.3.0 to 3.5.0
- maven-source-plugin: from 3.1.0 to 3.2.1
- maven-surefire-plugin: from 3.0.0-M7 to 3.0.0-M8
- maven-jar-plugin: from 3.2.2 to 3.3.0

### Why are the changes needed?
- maven-checkstyle-plugin
apache/maven-checkstyle-plugin@maven-checkstyle-plugin-3.2.0...maven-checkstyle-plugin-3.2.1

- maven-clean-plugin
3.2.0 include a new feature: apache/maven-clean-plugin#6
https://github.com/apache/maven-clean-plugin/releases/tag/maven-clean-plugin-3.2.0

- maven-dependency-plugin
apache/maven-dependency-plugin@maven-dependency-plugin-3.3.0...maven-dependency-plugin-3.5.0

- maven-source-plugin
apache/maven-source-plugin@maven-source-plugin-3.1.0...maven-source-plugin-3.2.1

- maven-surefire-plugin
apache/maven-surefire@surefire-3.0.0-M7...surefire-3.0.0-M8

- maven-jar-plugin
https://github.com/apache/maven-jar-plugin/releases/tag/maven-jar-plugin-3.3.0

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?

- Pass GitHub Actions
- Manually check these plugins work normally

Closes #39899 from LuciferYang/SPARK-42355.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants