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

Fix OOM on agents when parsing large xml test artifacts (Reverting all changes related to #1132 as this is breaking multiple things) #1747

Merged
merged 1 commit into from Dec 23, 2015

Conversation

Projects
None yet
3 participants
@jyotisingh
Copy link
Contributor

commented Dec 23, 2015

No description provided.

}
return builder.toString().trim();

This comment has been minimized.

Copy link
@ketan

ketan Dec 23, 2015

Member

Can we just add a close quietly in this.method for good measure?

This comment has been minimized.

Copy link
@jyotisingh

jyotisingh Dec 23, 2015

Author Contributor

IOUtils.closeQuietly looks harmless, but so did the original PR 😆 . The intent was to leave it in the form it was earlier. I can still add it if you think its necessary?
Weren't you off to sleep?

This comment has been minimized.

Copy link
@arvindsv

arvindsv Dec 23, 2015

Member

My usual thought about closing streams is: Whoever opens it closes it. At the same "level" (or method) that it is opened.

This comment has been minimized.

Copy link
@arvindsv

arvindsv Dec 23, 2015

Member

But I see there are some incorrect usages of readToEnd, like in LoggingConfUpdateTransformer.

@arvindsv

This comment has been minimized.

Copy link
Member

commented Dec 23, 2015

Seems ok to me.

$ git diff HEAD 15.2.0 -- base/src/com/thoughtworks/go/util/FileUtil.java
diff --git a/base/src/com/thoughtworks/go/util/FileUtil.java b/base/src/com/thoughtworks/go/util/FileUtil.java
index 02d3b08..6b84cf0 100644
--- a/base/src/com/thoughtworks/go/util/FileUtil.java
+++ b/base/src/com/thoughtworks/go/util/FileUtil.java
@@ -1,5 +1,6 @@
-/*
- * Copyright 2015 ThoughtWorks, Inc. *
+/*************************GO-LICENSE-START*********************************
+ * Copyright 2014 ThoughtWorks, Inc.
+ *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
  * You may obtain a copy of the License at
@@ -11,7 +12,7 @@
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
- */
+ *************************GO-LICENSE-END***********************************/

 package com.thoughtworks.go.util;

$ git diff HEAD 15.2.0 -- common/test-resources/unit/data/MinifiedJunitReport.xml
$ git diff HEAD 15.2.0 -- common/test/unit/com/thoughtworks/go/domain/UnitTestReportGeneratorTest.java
$ git diff HEAD 15.2.0 -- common/test/unit/com/thoughtworks/go/util/FileUtilTest.java
$ git diff HEAD 15.2.0 -- config/config-api/src/com/thoughtworks/go/domain/UnitTestReportGenerator.java
@jyotisingh

This comment has been minimized.

Copy link
Contributor Author

commented Dec 23, 2015

waiting for someone to click on the merge button if it looks about ok? 😄

arvindsv added a commit that referenced this pull request Dec 23, 2015

Merge pull request #1747 from jyotisingh/revert_all_changes_for_1132
Reverting all changes related to #1132 as this is breaking multiple things

@arvindsv arvindsv merged commit c0f91b2 into gocd:15.3 Dec 23, 2015

@ketan ketan added this to the Release 15.3.1 milestone Dec 24, 2015

@ketan ketan changed the title Reverting all changes related to #1132 as this is breaking multiple things Fix OOM on agents when parsing large xml test artifacts (Reverting all changes related to #1132 as this is breaking multiple things) Dec 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.