-
Notifications
You must be signed in to change notification settings - Fork 96
[JENKINS-37575] Keep track of how much content has been copied by writeLog even if the callable is interrupted #74
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -50,7 +50,7 @@ | |
| import java.util.logging.Logger; | ||
| import jenkins.MasterToSlaveFileCallable; | ||
| import org.apache.commons.io.IOUtils; | ||
| import org.jenkinsci.remoting.RoleChecker; | ||
| import org.apache.commons.io.output.CountingOutputStream; | ||
|
|
||
| import javax.annotation.CheckForNull; | ||
|
|
||
|
|
@@ -125,43 +125,40 @@ protected FileMonitoringController(FilePath ws) throws IOException, InterruptedE | |
|
|
||
| @Override public final boolean writeLog(FilePath workspace, OutputStream sink) throws IOException, InterruptedException { | ||
| FilePath log = getLogFile(workspace); | ||
| Long newLocation = log.act(new WriteLog(lastLocation, new RemoteOutputStream(sink))); | ||
| if (newLocation != null) { | ||
| LOGGER.log(Level.FINE, "copied {0} bytes from {1}", new Object[] {newLocation - lastLocation, log}); | ||
| lastLocation = newLocation; | ||
| return true; | ||
| } else { | ||
| return false; | ||
| CountingOutputStream cos = new CountingOutputStream(sink); | ||
| try { | ||
| log.act(new WriteLog(lastLocation, new RemoteOutputStream(cos))); | ||
| return cos.getByteCount() > 0; | ||
| } finally { // even if RemoteOutputStream write was interrupted, record what we actually received | ||
| long written = cos.getByteCount(); | ||
| if (written > 0) { | ||
| LOGGER.log(Level.FINE, "copied {0} bytes from {1}", new Object[] {written, log}); | ||
| lastLocation += written; | ||
| } | ||
| } | ||
| } | ||
| private static class WriteLog extends MasterToSlaveFileCallable<Long> { | ||
| private static class WriteLog extends MasterToSlaveFileCallable<Void> { | ||
| private final long lastLocation; | ||
| private final OutputStream sink; | ||
| WriteLog(long lastLocation, OutputStream sink) { | ||
| this.lastLocation = lastLocation; | ||
| this.sink = sink; | ||
| } | ||
| @Override public Long invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { | ||
| @Override public Void invoke(File f, VirtualChannel channel) throws IOException, InterruptedException { | ||
| long len = f.length(); | ||
| if (len > lastLocation) { | ||
| RandomAccessFile raf = new RandomAccessFile(f, "r"); | ||
| try { | ||
| try (RandomAccessFile raf = new RandomAccessFile(f, "r")) { | ||
| raf.seek(lastLocation); | ||
| long toRead = len - lastLocation; | ||
| if (toRead > Integer.MAX_VALUE) { // >2Gb of output at once is unlikely | ||
| throw new IOException("large reads not yet implemented"); | ||
| } | ||
| // TODO is this efficient for large amounts of output? Would it be better to stream data, or return a byte[] from the callable? | ||
| byte[] buf = new byte[(int) toRead]; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Still not a fan of the fact that we chose to do a complete buffer read (since potentially we could have a very large block of data). I'm itching to change this once these logging PRs are all integrated.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #60 (when active) renders this dead code, replaced by something that (potentially) streams content. |
||
| raf.readFully(buf); | ||
| sink.write(buf); | ||
| } finally { | ||
| raf.close(); | ||
| } | ||
| return len; | ||
| } else { | ||
| return null; | ||
| } | ||
| return 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.
Why is this line 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.
Because the speculation is obsolete—the PR is picking another approach entirely.