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

[3.8.x] [MNG-7476] Display a warning when an aggregator mojo locks other mojos executions #736

Merged
merged 2 commits into from May 30, 2022

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented May 12, 2022

This needs to be applied to 3.8.x, 3.9.x and master branches

@gnodet
Copy link
Contributor Author

gnodet commented May 12, 2022

Fwiw, I've raised #737 to make the console output adapt to the terminal width.

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.

This now interferes/depends on the other PR, do you want to keep this one as-is or use the old style for now until the message formatting is properly defined/discussed?

@gnodet
Copy link
Contributor Author

gnodet commented May 12, 2022

This now interferes/depends on the other PR, do you want to keep this one as-is or use the old style for now until the message formatting is properly defined/discussed?

I'd rather commit that one to 3.8.x and other branches asap and investigate the other PR for 3.9.x and master once this one is committed.

@gnodet gnodet changed the title [MNG-7476] Display a warning when an aggregator mojo is lock other mojos executions [3.8.x] [MNG-7476] Display a warning when an aggregator mojo is lock other mojos executions May 12, 2022
@michael-o
Copy link
Member

This now interferes/depends on the other PR, do you want to keep this one as-is or use the old style for now until the message formatting is properly defined/discussed?

I'd rather commit that one to 3.8.x and other branches asap and investigate the other PR for 3.9.x and master once this one is committed.

Alright, will retest tomorrow.

@gnodet gnodet force-pushed the MNG-7476 branch 2 times, most recently from e37727f to 1b2cbc4 Compare May 16, 2022 09:28
@gnodet
Copy link
Contributor Author

gnodet commented May 16, 2022

Fwiw, I've reviewed the original MNG-7433 issue and especially the reproducer and found out the issue was just a bug in the original project locking and not simply a missing warning. The lock was acquired too early, so that forked lifecycles could not obtain the lock for the project at all.

@michael-o michael-o self-requested a review May 16, 2022 19:08
@michael-o
Copy link
Member

@gnodet Can the reproducer be turned into an IT?

@gnodet
Copy link
Contributor Author

gnodet commented May 16, 2022

@gnodet Can the reproducer be turned into an IT?

I suppose. The failure would lead to a deadlock. is there a timeout that can be set in the invoker ?

@michael-o
Copy link
Member

@gnodet Can the reproducer be turned into an IT?

I suppose. The failure would lead to a deadlock. is there a timeout that can be set in the invoker ?

@slawekjaranowski Do you know this from top of your head?

@gnodet
Copy link
Contributor Author

gnodet commented May 17, 2022

@gnodet Can the reproducer be turned into an IT?

@michael-o apache/maven-integration-testing#162

@gnodet gnodet force-pushed the MNG-7476 branch 3 times, most recently from 4496c44 to 3786e53 Compare May 17, 2022 15:24
@gnodet gnodet changed the title [3.8.x] [MNG-7476] Display a warning when an aggregator mojo is lock other mojos executions [3.8.x] [MNG-7476] Display a warning when an aggregator mojo locks other mojos executions May 19, 2022
@slawekjaranowski
Copy link
Member

@gnodet Can the reproducer be turned into an IT?

I suppose. The failure would lead to a deadlock. is there a timeout that can be set in the invoker ?

@slawekjaranowski Do you know this from top of your head?

There is no implementation for timeouts in Verifier.

@michael-o
Copy link
Member

Moved message helper to #746.

@michael-o michael-o force-pushed the MNG-7476 branch 2 times, most recently from 66cbbb9 to a6e0854 Compare May 29, 2022 14:29
@michael-o
Copy link
Member

@gnodet Can the reproducer be turned into an IT?

I suppose. The failure would lead to a deadlock. is there a timeout that can be set in the invoker ?

@slawekjaranowski Do you know this from top of your head?

There is no implementation for timeouts in Verifier.

I have checked the source code of verifier. The lower level library supports timeouts in seconds, so we just need to wrap this with verifier. The JVM must be forked for this, so if timeout is > 0 and forkMode is auto, then autoforking must happen.

@michael-o
Copy link
Member

michael-o commented May 29, 2022

Polished commits, they are now subsequently, standalone testable (tested actually). As with previous releases I have created a JIRA issue which addresses the problem which in turn solves the regression. Immediately visible in the changelog. This PR works decently with the fixed IT PR. Only one open question needs to be answered by @gnodet. Will align IT as well. Thank you for the effort.

So this does not require squashing, but addresses two subsequent issues. Just need upporting to 3.9.x and master.

@michael-o
Copy link
Member

IT aligned and blessed.

@gnodet
Copy link
Contributor Author

gnodet commented May 30, 2022

@michael-o I've slightly refactored the MojoExecutor to cleanup the access to the mojos map.

@michael-o
Copy link
Member

@michael-o I've slightly refactored the MojoExecutor to cleanup the access to the mojos map.

This looks good and makes sense. Totally scoped in the ProjectLock. As far as I can see this commit shall be merged into "[MNG-7487]" and "[MNG-7476]" rebased on top of that.

@gnodet
Copy link
Contributor Author

gnodet commented May 30, 2022

@michael-o I've slightly refactored the MojoExecutor to cleanup the access to the mojos map.

This looks good and makes sense. Totally scoped in the ProjectLock. As far as I can see this commit shall be merged into "[MNG-7487]" and "[MNG-7476]" rebased on top of that.

Done

@michael-o michael-o self-requested a review May 30, 2022 09:20
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.

Fantastic, let's roll!

@gnodet gnodet merged commit 5c0d6b9 into apache:maven-3.8.x May 30, 2022
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