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.

Sign up for GitHub

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

Jump to bottom

[FIXED JENKINS-30909] Persist queue state after queue id counter increment #1865

Closed
wants to merge 1 commit into from
Closed

[FIXED JENKINS-30909] Persist queue state after queue id counter increment #1865

wants to merge 1 commit into from

Conversation

tfennelly
Copy link
Member

@tfennelly tfennelly added the needs-more-reviews Complex change, which would benefit from more eyes label Oct 12, 2015
@daniel-beck
Copy link
Member

Persisting the entire queue on every addition of a new item seems prohibitively expensive on busy instances / for long queues. Something similar to the nextBuildNumber file would probably be better.

@tfennelly
Copy link
Member Author

@daniel-beck It would be more expensive than the nextBuildNumber approach, but I've no idea how one could say it's prohibitive. Is XStream that bad?

@tfennelly
Copy link
Member Author

That said ... I've no prob with pulling that info out of the queue.xml and doing it the same as nextBuildNumber if people think that will make a big difference.

@tfennelly tfennelly closed this Oct 12, 2015
@tfennelly tfennelly reopened this Oct 12, 2015
oleg-nenashev
oleg-nenashev reviewed Oct 13, 2015
View reviewed changes
core/src/main/java/hudson/model/Queue.java
} finally {
// Persist the queue state in case Jenkins
// dies unexpectedly. JENKINS-30909
getInstance().save();
Copy link
Member

Choose a reason for hiding this comment

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

This method may fail with runtime exception if Jenkins is shutting down. Is it a potential case?

@oleg-nenashev
Copy link
Member

🐛
Seems you're going to save the queue twice on each scheduling. It causes a very significant overhead on large-scale instances with long build queues (we suffer from it even now).

I agree that the data should be persisted in a separate file

@tfennelly
Copy link
Member Author

Dumping this PR in favour of persisting it ala nextBuildNumber.

@tfennelly tfennelly closed this Oct 13, 2015
@tfennelly tfennelly removed the needs-more-reviews Complex change, which would benefit from more eyes label Oct 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tfennelly @daniel-beck @oleg-nenashev