Skip to content

Simplified PlaceholderTask #354

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

Merged
jglick merged 4 commits into from
Jan 30, 2024
Merged

Simplified PlaceholderTask #354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8ffd109
Simplified `PlaceholderTask`
jglick Jan 26, 2024
2017151
Logging from `RunningTasks.remove` https://github.com/jenkinsci/workf…
jglick Jan 26, 2024
d1a2aa2
`willContinue` implementation was wrong (failed to return false after…
jglick Jan 29, 2024
c52e84d
Test coverage for `willContinue`
jglick Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/main/java/org/jenkinsci/plugins/workflow/support/pickles/ExecutorPickle.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,10 @@ protected Executor tryResolve() throws Exception {
if (task instanceof ExecutorStepExecution.PlaceholderTask) {
ExecutorStepExecution.PlaceholderTask placeholder = (ExecutorStepExecution.PlaceholderTask)task;

// Non-null cookie means body has begun execution, i.e. we started using a node
// PlaceholderTask#getAssignedLabel is set to the Node name when execution starts
// Thus we're guaranteeing the execution began and the Node is now unknown.
// Theoretically it's safe to simply fail earlier when rehydrating any EphemeralNode... but we're being extra safe.
if (placeholder.getCookie() != null && Jenkins.get().getNode(placeholder.getAssignedLabel().getName()) == null ) {
if (placeholder.hasStarted() && Jenkins.get().getNode(placeholder.getAssignedLabel().getName()) == null ) {
Copy link
Member Author

@jglick jglick Jan 26, 2024

Choose a reason for hiding this comment

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

(just better encapsulation)

if (System.nanoTime() > endTimeNanos) {
Queue.getInstance().cancel(item);
owner.getListener().getLogger().printf("Killed %s after waiting for %s because we assume unknown agent %s is never going to appear%n",
Expand Down
4 changes: 1 addition & 3 deletions src/main/java/org/jenkinsci/plugins/workflow/support/steps/ExecutorStepDynamicContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ public final class ExecutorStepDynamicContext implements Serializable {

private static final long serialVersionUID = 1;

@NonNull ExecutorStepExecution.PlaceholderTask task;
final @NonNull ExecutorStepExecution.PlaceholderTask task;
final @NonNull String node;
final @NonNull String path;
final int depth;
Expand Down Expand Up @@ -97,8 +97,6 @@ void resume(StepContext context) throws Exception {
if (item == null) {
throw new IllegalStateException("queue refused " + task);
}
// Try to avoid having distinct a instance of PlaceholderTask here compared to any previously-scheduled task.
task = (ExecutorStepExecution.PlaceholderTask)item.task;
Copy link
Member Author

@jglick jglick Jan 26, 2024

Choose a reason for hiding this comment

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

Reverting this bit of #344 which should now be unnecessary: any instances in memory ought to be interchangeable at this point.

LOGGER.fine(() -> (result.isCreated() ? "scheduled " : " using already-scheduled ") + item + " for " + path + " on " + node);
TaskListener listener = context.get(TaskListener.class);
if (!node.isEmpty()) { // unlikely to be any delay for built-in node anyway
Expand Down
Loading