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-37575] Keep track of how much content has been copied by writeLog even if the callable is interrupted #74
Conversation
…teLog even if the callable is interrupted.
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.
AFAICT seems reasonable -- and I'd like to integrate and release this before the UTF-8 encoding change goes out (and then if all looks good, the new Gather APIs would follow that).
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? |
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.
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]; |
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 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.
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.
#60 (when active) renders this dead code, replaced by something that (potentially) streams content.
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.
@jglick What happens if encoding changes alter the number of bytes needed to represent the character content (likely to happen in the interactions with the UTF-8 encoding PRs I think)? It looks like we're counting at the output level, which would be after character set transcoding, but seeking by the same amount in the log file itself (original encoding). Or am I misunderstanding how that interaction will work?
|
@jglick What is the next step for this PR? |
@recampbell I am awaiting reviews. |
I needed to double check yet another interaction with #61 to make sure this wouldn't need amendment with the changes there but I think we're fine now. |
…nly what Handler.output actually received, in case of some exception halfway followed by a fresh agent connection.
See JENKINS-37575 where there is information to reproduce. This is dead code as of #60 + jenkinsci/workflow-durable-task-step-plugin#63 but may as well fix the bug so long as the old code path can be run at least with a kill switch.