-
-
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
[JENKINS-38313] - Introduce LogStorage API in the core #3575
[JENKINS-38313] - Introduce LogStorage API in the core #3575
Conversation
…bmissions from Elasticsearch
Hide launcher complexity from plugin.
…-task-logging-poc # Conflicts: # core/src/main/java/hudson/model/AbstractProject.java # core/src/main/java/hudson/model/Job.java # core/src/main/java/hudson/model/Run.java
# Conflicts: # core/src/main/java/hudson/model/Job.java # core/src/main/java/hudson/model/Run.java
…sisted for Runs + extend API
…ation to External logging API
…er + documentation
Test failures are unrelated - UC issues |
@jenkinsci/cloud-native-sig I have addressed all comments in this change. Tests have been also added. IMO it is ready for review. |
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.
from my limited knowledge
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 main questions are about the relationship to jenkinsci/workflow-job-plugin#27 and specifically when and how the log storage is selected; see https://github.com/jenkinsci/workflow-api-plugin/pull/17/files#diff-1d162ef4f3f75e09c30e1f8e52e36b01R118.
@@ -530,6 +531,10 @@ protected Launcher createLauncher(@Nonnull BuildListener listener) throws IOExce | |||
final Node currentNode = getCurrentNode(); | |||
Launcher l = currentNode.createLauncher(listener); | |||
|
|||
// Produce correct logger | |||
// TODO: Consider merging with create Launcher | |||
l = getLogStorage().decorateLauncher(l, getBuild(), currentNode, listener); |
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.
You should not need this.
* @throws InterruptedException Was interrupted while decorating listeners | ||
*/ | ||
@Nonnull | ||
public Launcher decorateLauncher(@Nonnull Launcher original, |
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.
Remove. I cannot foresee any reason why we would ever need this, even with external sinks implemented via external processes—these should not be touching the user process being launched.
@@ -289,6 +286,12 @@ | |||
*/ | |||
private @CheckForNull ArtifactManager artifactManager; | |||
|
|||
/** | |||
* Log storage associated with this build, if any. | |||
* @since TODO |
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.
No need for @since
on private stuff, only public APIs.
@@ -345,6 +348,10 @@ public void reload() throws IOException { | |||
LOGGER.log(WARNING, "reload {0} @{1} with anomalous state {2}", new Object[] {this, hashCode(), state}); | |||
} | |||
|
|||
if (logStorage == null) { | |||
logStorage = new CompatFileLogStorage(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.
Rather keep it null and just return CompatFileLogStorage
on demand so we do not touch the serial form unless you are actually using a nondefault implementation. Compare Run.getArtifactManager()
vs. Run.artifactManager
. You might need to introduce a pickLogStorage()
correspondingly.
/** | ||
* Returns {@code true} if the log file is no longer being updated. | ||
*/ | ||
public boolean isLoggingFinished(); |
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.
Consider isLoggingComplete
(or isLoggingCompleted
) by analogy with LargeText
parameters?
@ExportedBean | ||
public abstract class LogStorage<T extends Loggable> { | ||
|
||
protected transient T loggable; |
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.
Better to delete this and pass a T
to any method which might need it. Make this an interface
while you are at it.
* @throws InterruptedException Operation was interrupted | ||
*/ | ||
@Nonnull | ||
public abstract AnnotatedLargeText<T> overallLog() 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.
Rename to getLogText
for consistency with Run
.
@@ -1435,39 +1467,60 @@ public String toString() { | |||
return rawF; |
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.
Consider removing the historical gzip hacks and at best make this into a LogStorageFactory
that detects log.gz
existence for compatibility reasons (but never does this for new builds).
* @since TODO | ||
*/ | ||
@Restricted(Beta.class) | ||
public class NoopLogStorage extends 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.
Still in use?
* @since TODO | ||
*/ | ||
@Restricted(Beta.class) | ||
public abstract class StreamLogStorage extends 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.
Maybe just delete it, since the hard parts are the read methods anyway; createBuildListener
should become trivial.
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.
A tip for killing StreamLogStorage
.
|
||
// Global log filter | ||
for (ConsoleLogFilter filter : ConsoleLogFilter.all()) { | ||
logger = filter.decorateLogger(build, logger); |
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.
Suggest moving this to a protected
method in RunExecution
…
final Job<?, ?> project = build.getParent(); | ||
|
||
// Project specific log filters | ||
if (project instanceof BuildableItemWithBuildWrappers && build instanceof AbstractBuild) { |
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.
…and then override that in AbstractBuildExecution
, cleaning this mistake up at last.
WorkflowRun
does not yet support ConsoleLogFilter.all()
but that can be dealt with separately: https://github.com/jenkinsci/workflow-job-plugin/pull/27/files#diff-68f690dbd3fa2d9fb89d6af3f8fd7216R218
* @return An input stream from the log file. | ||
* If the log file does not exist, the error message will be returned to the output. | ||
* @since 1.349 | ||
*/ | ||
public @Nonnull InputStream getLogInputStream() throws IOException { | ||
File logFile = getLogFile(); |
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 more current ideas about these methods, see overloads in WorkflowRun
edited during jenkinsci/workflow-job-plugin#114 etc.
This was abandoned and does not need to be left open. |
Version of #3557, but with a single LogStorage as proposed by @jglick . The previous implementation had separate
LoggingMethod
andLogBrowser
implementation directly in the core, but with this change I will need to move it to External Logging API.Proposed Changelog Entries
Submitter checklist
* Use the
Internal:
prefix if the change has no user-visible impact (API, test frameworks, etc.)@carlossg @jenkinsci/code-reviewers I will appreciate feedback about the preferred approach