-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Simplify persistence design for temporarily offline status #9855
base: master
Are you sure you want to change the base?
Conversation
There were complex code to synchronize transient state of Computer from Node. But since any temporarily offline cause is being set by a user and should be persisted in a node, the Computer should actually delegate to Node instead of trying to synchronize it
getNodeOrDie().setTemporaryOfflineCause(temporarilyOfflineCause); | ||
if (temporarilyOfflineCause != null) { | ||
Listeners.notify(ComputerListener.class, false, l -> l.onTemporarilyOffline(this, temporarilyOfflineCause)); | ||
} else { | ||
Listeners.notify(ComputerListener.class, false, l -> l.onTemporarilyOnline(this)); | ||
} |
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 field offlineCause
is only for technical offline cause now. temporarilyOfflineCause
only exists in Node
now.
var temporaryOfflineCause = getNodeOrDie().getTemporaryOfflineCause(); | ||
return temporaryOfflineCause == null ? offlineCause : temporaryOfflineCause; |
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.
Retaining previous behaviour : if a temporary offline cause is defined, from outside PoV, it replaces the technical offline cause.
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.
getNodeOrDie
is probably wrong. Otherwise looks good.
return nodeName == null ? Jenkins.get() : Jenkins.get().getNode(nodeName); | ||
var node = nodeName == null ? Jenkins.get() : Jenkins.get().getNode(nodeName); | ||
if (node == null) { | ||
throw new IllegalStateException("Can't set a temporary offline cause if the node has been removed"); |
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 think this nullness check needs to be moved up into the callers, since some are just reading the status, not trying to set it, right?
@@ -620,7 +625,7 @@ public BuildTimelineWidget getTimeline() { | |||
@Exported | |||
@Override | |||
public boolean isOffline() { | |||
return temporarilyOffline || getChannel() == null; | |||
return getNodeOrDie().isTemporarilyOffline() || getChannel() == null; |
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.
For example this will throw an ISE from the whole api/json
endpoint of a computer corresponding to a recently removed permanent agent, which is a legal situation.
return getNodeOrDie().isTemporarilyOffline() || getChannel() == null; | |
var node = getNode(); | |
return node != null && node.isTemporarilyOffline() || getChannel() == null; |
getNodeOrDie
could probably just be removed, and callers of getNode
should decide how best to handle nullness; even for an offline setter, an ISE seems inappropriate—either just ignore the request (harmless, soon to be meaningless) or return a 404 or whatever.
There were complex code to synchronize transient state of
Computer
fromNode
and vice versa.But since any temporarily offline cause is being set by a user and should be persisted in a
Node
, theComputer
should actually delegate toNode
instead of trying to synchronize its state.See JENKINS-XXXXX.
Testing done
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist