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

Save build.xml during CpsFlowExecution.suspendAll #923

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jglick
Copy link
Member

@jglick jglick commented Aug 24, 2024

I suspect this will fix a hard-to-reproduce bug in FlowHead deserialization. I have not managed to reproduce an error yet in a functional test, but I have found that build.xml including its list of heads is only getting saved rather incidentally from the asynchronous mode of notifyListeners, even though FlowNodes have gotten saved earlier and program.dat has been saved. If I insert some artificial delays into notifyNewHead and run a restart test involving parallel, sometimes build.xml is out of date after Jenkins shuts down, causing weird effects like the iota being off after restart, or block start nodes being omitted from the graph.

@jglick
Copy link
Member Author

jglick commented Aug 24, 2024

Did not in fact fix the problem.

@jglick jglick closed this Aug 24, 2024
@jglick jglick deleted the CpsFlowExecution-save branch August 24, 2024 11:39
@jglick jglick restored the CpsFlowExecution-save branch August 24, 2024 15:59
@jglick jglick reopened this Aug 24, 2024
@jglick
Copy link
Member Author

jglick commented Aug 24, 2024

The immediate problem was #925, but even with that addressed, there seem to be random problems finding stored FlowNodes, so something like this may still be necessary.

Comment on lines +1663 to +1673
if (cpsExec.owner != null) {
try {
Queue.Executable exec = cpsExec.owner.getExecutable();
if (exec instanceof Saveable) {
LOGGER.fine(() -> "saving " + exec);
((Saveable) exec).save();
}
} catch (IOException x) {
LOGGER.log(Level.WARNING, "failed to save " + cpsExec, x);
}
}
Copy link
Member Author

@jglick jglick Oct 13, 2024

Choose a reason for hiding this comment

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

or perhaps just

Suggested change
if (cpsExec.owner != null) {
try {
Queue.Executable exec = cpsExec.owner.getExecutable();
if (exec instanceof Saveable) {
LOGGER.fine(() -> "saving " + exec);
((Saveable) exec).save();
}
} catch (IOException x) {
LOGGER.log(Level.WARNING, "failed to save " + cpsExec, x);
}
}
cpsExec.saveOwner();

though OTOH does

not already do just this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant