Skip to content
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

[Fixed JENKINS-13655] Enable transparent log decompression support #549

Closed
wants to merge 3 commits into from
Closed

[Fixed JENKINS-13655] Enable transparent log decompression support #549

wants to merge 3 commits into from

Conversation

HedAurabesh
Copy link
Contributor

This change enables use of the Stapler transparent decompression
support that was added by another pull-request to the Stapler project.

This means, that this change will not compile when the
changes to Stapler are not applied, as the new "LargeText"
constructor will not be found.

Additionally, the console.jelly was modified to make use of the
stream instead of the raw file, which is necessary to get the
correct uncompressed size of the file for skipping bytes.

This submission fixes the following bugs in the JIRA bug tracker:
[JENKINS-2551]
[JENKINS-10400]
[JENKINS-13655]

Signed-off-by: Martin Schroeder martin.h.schroeder@intel.com

This only works if the "transparent GZIP support" patch has been
applied against Stapler. Otherwise, this patch will not compile
as the new "LargeText" constructor will not be found.

Additionally, the console.jelly was modified to make use of the
stream instead of the raw file, which is necessary to get the
correct uncompressed size of the file for skipping bytes.

[JENKINS-2551]
[JENKINS-10400]
[JENKINS-13655]

Signed-off-by: Martin Schroeder <martin.h.schroeder@intel.com>
@HedAurabesh
Copy link
Contributor Author

The link to the associated Stapler pull request is:

jenkinsci/stapler#10

@jglick
Copy link
Member

jglick commented Aug 29, 2012

JENKINS-10400 is currently marked fixed. Should it be reopened, and does your pull request claim to fix it?

@jglick
Copy link
Member

jglick commented Aug 29, 2012

Will there be any effect on a client of the REST API /job/whatever/123/progressiveLog?start=0? (The NetBeans IDE uses this to stream logs from running or completed builds, and there are probably other clients.) I doubt the REST API would be affected even when the log is compressed, but it would be good to test this.

@HedAurabesh
Copy link
Contributor Author

JENKINS-10400 is currently marked fixed. Should it be reopened, and does your pull request claim to fix it?

It replaces the fix for JENKINS-10400, as that change introduced the "fix" in Jenkins that checks if a "log.gz" exists, and if so, return a GZIPOutputStream instead of a FileOutputStream.

This fix was subsequently broken by the introduction of the LineFormatter API (if I remember correctly) which allows plugins like the "Timestamper" to annotate the lines. These annotations were dumped as GZIP+Base64 encoded blobs into the log file itself.

The original fix for JENKINS-10400 caused these blobs to be displayed as-is in Base64. With this patch; Stapler does transparent decompression for Jenkins, so that the latter can properly decode these blobs as if the file was uncompressed.

@HedAurabesh
Copy link
Contributor Author

Will there be any effect on a client of the REST API [...] ?

No.

This patch only works on files that are already GZIPed and does not compress anything on its own. While the log is generated, it remains uncompressed; as such, the progressive log API works fine.

Only if you afterwards compress the logs externally -- either via a cronjob or a periodic job inside Jenkins -- does this patch cause any alteration in the behaviour of Stapler and Jenkins.

@jglick
Copy link
Member

jglick commented Aug 29, 2012

Well what I was asking was whether progressiveLog will still work after the log is compressed. If not, we have a compatibility issue.

@HedAurabesh
Copy link
Contributor Author

Thanks, now I understand you. :)

I've just tested it on our internal productive Jenkins instance that has the patches applied. I tested the following calls, both with a compressed and uncompressed log file. Both cases returned the exactly same output:

http://[server]/job/[jobName]/[id]/progressiveLog
http://[server]/job/[jobName]/[id]/progressiveLog?start=0
http://[server]/job/[jobName]/[id]/progressiveLog?start=10000

The only change is that the "start" option will now need to decompress the file until that position; instead of the entire operation being just a simple "seek" operation.

This is by the way also the reason why I decided to not implement any online compression of the log.

@jglick
Copy link
Member

jglick commented Aug 29, 2012

In other words, progressiveLog?start=10000 will just be a bit slower. That is to be expected, and it probably would not matter for a completed build: you would typically be starting with progressiveLog?start=0, which should not set X-More-Data: true, so that one HTTP request should download the full log anyway. Thanks for checking this.

I agree that compressing the log of a build in progress could lead to poor performance.

Signed-off-by: Martin Schroeder <martin.h.schroeder@intel.com>
@HedAurabesh
Copy link
Contributor Author

Hi everyone.

Any updates on when this pull request will be merged? The associated pull request to Stapler was already merged 2 weeks ago, so there should be no problems:

jenkinsci/stapler#10

Thanks!

@HedAurabesh
Copy link
Contributor Author

Hi again.

I just took a look at the changes in the Jenkins and Stapler git repos. The changes that this pull request needed in Stapler have been merged into its mainline starting with v1.196.

Jenkins makes use of these changes since its v1.482 release; where the dependency on Stapler was moved from v1.195 to v1.197. As such this pull request can be merged immediately as all the necessary dependencies are now fulfilled.

Thanks!

@jglick
Copy link
Member

jglick commented Oct 5, 2012

There is some sort of merge conflict with trunk; you should fetch origin/master and merge it into your branch to resolve.

Will probably leave it to @kohsuke to review & apply, but this all looks reasonable to me (have not personally tested).

This resolves a tiny merge conflict on a changed comment.

Signed-off-by: Martin Schroeder <martin.h.schroeder@intel.com>
@HedAurabesh
Copy link
Contributor Author

Hi everyone.

I've merged in all the changes from the 1.485 tag on origin/master. All this does is to fix a single small conflict on a single comment, where Darek Ostolski added his company name to the licencing comment.

Hopefully this merge will not break things even further. :P

@HedAurabesh
Copy link
Contributor Author

Closed in favour of the following pull request, as the branch that this request was based on has become unmergeable.

#586

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants