-
Notifications
You must be signed in to change notification settings - Fork 65
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
[JEP-210] Log handling rewrite #15
Conversation
Not going to bother deploying upstream snapshots at this stage, so CI failures are expected. |
… to be usable from LogAction. (Core-defined notes do use Object, explicitly or implicitly via rawtypes.)
…al LogActionImpl. Includes further encapsulation than that started in jenkinsci#10. Also applying ConsoleLogFilter in new mode.
…epContext to AnnotatedLogAction.
Using jitpack for jenkinsci/workflow-api-plugin#17, and picking up jenkinsci/maven-hpi-plugin#46.
…p a deployed snapshot.
Also using jitpack and linking to jenkinsci/maven-hpi-plugin#46 & jenkinsci/workflow-api-plugin#17 & jenkinsci/workflow-support-plugin#15.
@jglick Does this fix the issue we discussed months ago where it requires reading the entire logfile to retrieve the fragment for a single step? |
That has since been refactored into jenkinsci/workflow-api-plugin#17 where it now lives in Rendering speed is anyway not a priority for JEP-210, compared to optimizing writing during the build. (discussion) |
* A marker for a node which had some log text. | ||
*/ | ||
@Restricted(NoExternalUse.class) // for use from DefaultStepContext only | ||
public class AnnotatedLogAction extends LogAction implements FlowNodeAction, PersistentAction { |
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.
This should be renamed.
… had used LogAction, it must continue to do so (now teeing to the overall log).
src/main/java/org/jenkinsci/plugins/workflow/support/actions/LogStorageAction.java
Show resolved
Hide resolved
public static @Nonnull TaskListener listenerFor(@Nonnull FlowNode node, @CheckForNull ConsoleLogFilter filter) throws IOException, InterruptedException { | ||
FlowExecutionOwner owner = node.getExecution().getOwner(); | ||
if (LogActionImpl.isOld(owner) || node.getAction(LogActionImpl.class) != null) { | ||
return LogActionImpl.stream(node, filter); |
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.
Probably the !FlowNode.active
closing logic from LogActionImpl
should be moved here—for some implementations it may be significant (though not for FileLogStorage
). Also rather than PrintStream.close
it should check for TaskListener & AutoCloseable
. And the expected close behavior for both the overall and step logs should be documented in LogStorage
.
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.
(resolved)
…support LogStorageAction, and handling TaskListener & AutoCloseable.
src/main/java/org/jenkinsci/plugins/workflow/support/actions/LogStorageAction.java
Show resolved
Hide resolved
@Override public void onNewHead(FlowNode newNode) { | ||
if (!node.isActive()) { | ||
node.getExecution().removeListener(this); | ||
LOGGER.log(Level.FINE, "closing log for {0}", node.getDisplayFunctionName()); |
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 a good time, try to disentangle what all the xxFunctionName/xxDisplayName function for a FlowNode do and which to use when.
And by a "good time" I mean "if you're a masochist."
@@ -97,6 +99,33 @@ | |||
} | |||
} | |||
|
|||
private synchronized TaskListener getListener() throws IOException, InterruptedException { |
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.
Synchronizing is /probably/ the right move here but makes me nervous we're going to hit threading issues (as has happened with analogous changes in WorkflowRun with bottlenecks and deadlocks). Nothing I can point to explicitly wrong here, but something to keep a close eye on.
…e is expected, send it to the overall build log not the step log. This allows us to call context.get(EnvVars.class) even on a FlowNode which has not yet been attached to the graph.
…e is expected, send it to the overall build log not the step log. This allows us to call context.get(EnvVars.class) even on a FlowNode which has not yet been attached to the graph. (cherry picked from commit d5d1f46)
@@ -62,17 +67,14 @@ | |||
if (key == EnvVars.class) { | |||
Run<?,?> run = get(Run.class); | |||
EnvironmentAction a = run == null ? null : run.getAction(EnvironmentAction.class); | |||
EnvVars customEnvironment = a != null ? a.getEnvironment() : run.getEnvironment(get(TaskListener.class)); | |||
EnvVars customEnvironment = a != null ? a.getEnvironment() : run.getEnvironment(getExecution().getOwner().getListener()); |
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.
Amended in #62 FTR.
Downstream of jenkinsci/workflow-api-plugin#17. Subsumes #75.
JEP-210
way to efficiently get all output from a branchworkflow-job
AnnotatedLogAction.getLogText
AnnotatedLogAction.strip