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
[JENKINS-34138] Fix maven installs from stepping on each other #3042
Conversation
… step on each other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix LGTM, needs minor patch in the tests
@@ -356,4 +354,18 @@ public void parametersReferencedFromPropertiesShouldRetainBackslashes() throws E | |||
assertTrue("Properties should always be injected, even when build variables injection is enabled", | |||
log.contains("-DTEST_PROP1=VAL1") && log.contains("-DTEST_PROP2=VAL2")); | |||
} | |||
|
|||
@Test public void checkMavenInstallationEquals() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add @Issue("JENKINS-34138")
assertTrue(maven.equals(maven2)); | ||
} | ||
|
||
@Test public void checkMavenInstallationNotEquals() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
public int hashCode() { | ||
int result = getName() != null ? getName().hashCode() : 0; | ||
result = 31 * result + (getHome() != null ? getHome().hashCode() : 0); | ||
//result = 31 * result + (getProperties() != null ? getProperties().hashCode() : 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no strong opinion about that. If getHome()
overlaps, there is likely a problem anyway
@jenkinsci/code-reviewers @aheritier WDYT? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm doubtful @oleg-nenashev
I agree with you if the Home is the same we are doomed
If the have different names (and potentially different installers/versions we are completely doomed :(
This PR is good (=better than the current state) but we should perhaps start by verifying the home (if set) and then the name for a better behavior. WDYT ?
@t-hall would you be able to update the PR accordingly? |
So you are asking to change the order from checking name first to check home first? |
Test failure is unrelated, I'd guess: |
retriggering CI |
@aheritier PTAL |
@aheritier ping. I am going to merge it next week. if there is no negative feedback. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @oleg-nenashev
The reorder of imports in MavenTest are probably useless but let's go
retriggering CI |
Reorder is fine, there is no colliding PRs |
* [JENKINS-34138] Adding equals/hashCode methods so that installs don't step on each other * [JENKINS-34138] Added issue reference to unit tests * [JENKINS-34138] - changed the order of equals / hashcode (cherry picked from commit d688c15)
When multiple maven projects are launched on a new node the installs can step on each other. There is locking inside InstallerTranslator but because maven does not have equals/hashcode method and the maven installs are different instances they can run concurrently. This only works for the JDK because the same instance is used (populated from a global location on the abstract project).
See JENKINS-34138.
Proposed changelog entries
Submitter checklist
Desired reviewers
@mention
@jenkinsci/code-reviewers