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

Resume stashing megawars #1992

Merged
merged 2 commits into from Apr 22, 2023
Merged

Resume stashing megawars #1992

merged 2 commits into from Apr 22, 2023

Conversation

jglick
Copy link
Member

@jglick jglick commented Apr 21, 2023

Reverts the essence of #1955, though a literal revert proved impractical since #1955 cleaned up stuff like MAVEN_ARGS usage, there were some minor subsequent changes, and mainly because #1970 made subtle changes I am not familiar with and do not wish to mess with. Instead keeping the Bash scripts as they were and reintroducing a simplified stash system just for CI. Requested by jenkins-infra/helpdesk#3496 (comment) due to #1969 (comment).

@jglick jglick added the chore Reduces future maintenance label Apr 21, 2023
@jglick jglick requested review from dduportal and a team April 21, 2023 20:09
Jenkinsfile Outdated
@@ -58,6 +58,9 @@ stage('prep') {
}
}
}
lines.each {
stash name: it, includes: "pct.sh,excludes.txt,target/pct.jar,target/megawar-$it.war"
Copy link
Member Author

Choose a reason for hiding this comment

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

99% of the stash is the megawar (now that PCT has been slimmed down considerably), so it did not seem worth the bother to separate the generic from line-specific contents. Anyway on S3 there is some fixed overhead per blob operation, and each Pipeline step adds a bit of overhead as well, so minimizing the count is probably the better choice. Also this is more legible I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

make sense 👍

@jglick
Copy link
Member Author

jglick commented Apr 21, 2023

Passed in 85m, good.

@dduportal are we ready to go here?

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

mainly because #1970 made subtle changes I am not familiar with and do not wish to mess with

I recommend you become familiar with them.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

Seems to leave the repository in a messier state than prior to #1955 by not fully reverting all of #1955. The decision about whether to revert or retain a given portion of #1955 should be driven by technical considerations, not one's desire to mess with or not mess with changes one does not (yet?) understand.

@jglick
Copy link
Member Author

jglick commented Apr 21, 2023

#1955 made certain simplifications along with extracting pieces of functionality to separate scripts, so I do not see any particular need to revert more. Also it is far from settled whether ultimately we will wish to use stashes or rebuild the megawar in different branches. Since this change is small, I see no reason to delay it; if someone wishes to devise some further simplifications to some scripts after ci.jenkins.io performance is stable, that can be done then.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

I do not see any particular need to revert more.

You do not see any particular need to revert more because you have not looked. Why would you write something like

changes I am not familiar with and do not wish to mess with

when those changes are relevant to this current PR? Why have you not looked?

#1955 left behind a bit of a mess, including apparently moving pct-work from its expected target/local-test/pct-work to pct-work, a regression that still remains in this PR and has affected others who work on this repository.

I am (again) requesting a systematic evaluation of the revert of #1955, which requires becoming familiar with the subsequent changes to these same files. This is your responsibility as someone who contributes code to a repository that other people also work on. If the portion of #1955 was valuable, it should be retained. If the portion of #1955 is no longer necessary or buggy, it should be removed.

@jglick
Copy link
Member Author

jglick commented Apr 21, 2023

moving pct-work from its expected target/local-test/pct-work to pct-work, a regression that […] has affected others

If there is a specific problem it can be addressed.

@basil
Copy link
Member

basil commented Apr 22, 2023

Note that I asked "Why would you write something like […] when those changes are relevant to this current PR?" and "Why have you not looked?" @jglick did not respond.

Copy link
Contributor

@dduportal dduportal left a comment

Choose a reason for hiding this comment

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

Thanks @jglick ! That unblocks the infra team working on stability, performance and costs of ci.jenkins.io.

LGTM

@dduportal
Copy link
Contributor

Merging to unblock the work on #1969

@dduportal dduportal merged commit 2a822dc into jenkinsci:master Apr 22, 2023
238 checks passed
@jglick jglick deleted the stashback branch April 25, 2023 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Reduces future maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants