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

changes to overcome performance issues with huge fitnesse result files #10

Merged
merged 5 commits into from Dec 12, 2013

Conversation

Projects
None yet
4 participants
@ricointersys
Copy link
Contributor

ricointersys commented Dec 5, 2013

(several MP)
fitnesse_change_proposed

Hi,
our company had an issue with performance of this plugin due to huge fitnesse results. Since the fitnesse results are stored in build.xml this file had a size of several MB. Details are explained here: https://issues.jenkins-ci.org/browse/JENKINS-13936.

We overcome the issue as follows:

  • do not store the fitnesse-results in build.xml, put them in separate files (in same directory as build.xml)
  • in addition we added a new column to directly jump to the fitnesse history page

We would like to give back the changes if you are interested in. there is still some work to do

  • update test cases
  • documentation

Please let me know if you are interested
Rico

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented Dec 5, 2013

plugins » fitnesse-plugin #10 UNSTABLE
Looks like there's a problem with this pull request

@ricointersys

This comment has been minimized.

Copy link
Contributor Author

ricointersys commented Dec 5, 2013

Hi,
as stated in the pull request, if you are interested in the changes I will fix the tests as well.
Regards
Rico

Von: CloudBees pull request builder plugin [mailto:notifications@github.com]
Gesendet: Donnerstag, 5. Dezember 2013 11:26
An: jenkinsci/fitnesse-plugin
Cc: Rico Gaudlitz
Betreff: Re: [fitnesse-plugin] changes to overcome performance issues with huge fitnesse result files (#10)

plugins » fitnesse-plugin #10https://jenkins.ci.cloudbees.com/job/plugins/job/fitnesse-plugin/10/ UNSTABLE
Looks like there's a problem with this pull request


Reply to this email directly or view it on GitHubhttps://github.com//pull/10#issuecomment-29886703.

@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Dec 5, 2013

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

logger.println("all-content: " + pageCounts.getAllContents().size());
logger.println("resultsFile: " + getFitnessePathToXmlResultsIn());
// logger.println("content: " +
// pageCounts.getAllContents().values().iterator().next());

This comment has been minimized.

@lessonz

lessonz Dec 6, 2013

Member

Let's not leave commented-out code.

// Close the output stream
out.close();
iCount.contentFile = fileName;
} catch (Exception e) {// Catch exception if any

This comment has been minimized.

@lessonz

lessonz Dec 6, 2013

Member

Why catch raw Exception? This may also not ensure out is closed.

}
this.parentResults.getName();
// return getPageCounts().content;
// return "<b>hello world</b>";

This comment has been minimized.

@lessonz

lessonz Dec 6, 2013

Member

Please remove these two lines.

@lessonz

This comment has been minimized.

Copy link
Member

lessonz commented Dec 6, 2013

I think this would be a good addition to the project. Thanks for your efforts. Yes, please correct the unit tests. Also, having only had a chance to skim the code, why remove so much envVar references?

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented Dec 6, 2013

plugins » fitnesse-plugin #11 SUCCESS
This pull request looks good

@ricointersys

This comment has been minimized.

Copy link
Contributor Author

ricointersys commented Dec 6, 2013

Thanks for the review.
I did the requested updates and provided some documentation. Hope it is helpful.

I’d like to raise your attention to some ugly detail. To restore the saved fitnesse results (which are now in separate files) I had to store the filename (with directory) in the Jenkins-result. Is there a better way to retrieve the path to the current build in hudson.plugins.fitnesse.ResultsDetails.getDetailsHtml()?

why remove so much envVar references?
Sorry I do not get your question. I hope that my changes didn’t touch any envVar references. Could you please point me to the code?

Best regards
Rico

Von: lessonz [mailto:notifications@github.com]
Gesendet: Freitag, 6. Dezember 2013 06:21
An: jenkinsci/fitnesse-plugin
Cc: Rico Gaudlitz
Betreff: Re: [fitnesse-plugin] changes to overcome performance issues with huge fitnesse result files (#10)

I think this would be a good addition to the project. Thanks for your efforts. Yes, please correct the unit tests. Also, having only had a chance to skim the code, why remove so much envVar references?


Reply to this email directly or view it on GitHubhttps://github.com//pull/10#issuecomment-29964264.

@@ -73,17 +71,6 @@ private String getOption(String key, String valueIfKeyNotFound) {
return valueIfKeyNotFound;
}

private String getOption(String key, String valueIfKeyNotFound, EnvVars environment) {

This comment has been minimized.

@lessonz

lessonz Dec 8, 2013

Member

This method, and calls to it, are what I was referring to with regards to your removing EnvVars references.

This comment has been minimized.

@ricointersys

ricointersys Dec 9, 2013

Author Contributor

Just seen the changes. This is maybe an issue with the Fork I created. Those changes are by mistake. Shall I restore the previous version?
Sorry
Rico

Von: lessonz [mailto:notifications@github.com]
Gesendet: Sonntag, 8. Dezember 2013 07:03
An: jenkinsci/fitnesse-plugin
Cc: Rico Gaudlitz
Betreff: Re: [fitnesse-plugin] changes to overcome performance issues with huge fitnesse result files (#10)

In src/main/java/hudson/plugins/fitnesse/FitnesseBuilder.java:

@@ -73,17 +71,6 @@ private String getOption(String key, String valueIfKeyNotFound) {

  return valueIfKeyNotFound;

 }
  • private String getOption(String key, String valueIfKeyNotFound, EnvVars environment) {

This method, and calls to it, are what I was referring to with regards to your removing EnvVars references.


Reply to this email directly or view it on GitHubhttps://github.com//pull/10/files#r8183926.

This comment has been minimized.

@lessonz

lessonz Dec 10, 2013

Member

Yes, please try to ensure only the files you intended to change are actually modified. Thanks!

This comment has been minimized.

@ricointersys

ricointersys Dec 10, 2013

Author Contributor

Compared all files now with the history. Hope that I reverted all unintentional changes.

Von: lessonz [mailto:notifications@github.com]
Gesendet: Dienstag, 10. Dezember 2013 07:12
An: jenkinsci/fitnesse-plugin
Cc: Rico Gaudlitz
Betreff: Re: [fitnesse-plugin] changes to overcome performance issues with huge fitnesse result files (#10)

In src/main/java/hudson/plugins/fitnesse/FitnesseBuilder.java:

@@ -73,17 +71,6 @@ private String getOption(String key, String valueIfKeyNotFound) {

  return valueIfKeyNotFound;

 }
  • private String getOption(String key, String valueIfKeyNotFound, EnvVars environment) {

Yes, please try to ensure only the files you intended to change are actually modified. Thanks!


Reply to this email directly or view it on GitHubhttps://github.com//pull/10/files#r8223222.

This comment has been minimized.

@lessonz

lessonz Dec 11, 2013

Member

I'm sorry, but I don't see your most recent changes. Perhaps you forgot to push them?

This comment has been minimized.

@ricointersys

ricointersys Dec 11, 2013

Author Contributor

That is right, I forgot to push it … . Did it now.

Von: lessonz [mailto:notifications@github.com]
Gesendet: Mittwoch, 11. Dezember 2013 07:07
An: jenkinsci/fitnesse-plugin
Cc: Rico Gaudlitz
Betreff: Re: [fitnesse-plugin] changes to overcome performance issues with huge fitnesse result files (#10)

In src/main/java/hudson/plugins/fitnesse/FitnesseBuilder.java:

@@ -73,17 +71,6 @@ private String getOption(String key, String valueIfKeyNotFound) {

  return valueIfKeyNotFound;

 }
  • private String getOption(String key, String valueIfKeyNotFound, EnvVars environment) {

I'm sorry, but I don't see your most recent changes. Perhaps you forgot to push them?


Reply to this email directly or view it on GitHubhttps://github.com//pull/10/files#r8258257.

@lessonz

This comment has been minimized.

Copy link
Member

lessonz commented Dec 8, 2013

Please understand I only relatively recently took over this plugin. I am only responsible for about a grand total of 5 lines of the code base so far. With regards to retrieving the path to the current build, this is something I'd have to look into further.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

cloudbees-pull-request-builder commented Dec 11, 2013

plugins » fitnesse-plugin #12 SUCCESS
This pull request looks good

lessonz added a commit that referenced this pull request Dec 12, 2013

Merge pull request #10 from ricointersys/master
changes to overcome performance issues with huge fitnesse result files

@lessonz lessonz merged commit 5834f5f into jenkinsci:master Dec 12, 2013

@lessonz

This comment has been minimized.

Copy link
Member

lessonz commented Dec 12, 2013

Thanks so much for working through the issues. It's greatly appreciated.

@ricointersys

This comment has been minimized.

Copy link
Contributor Author

ricointersys commented Dec 12, 2013

Thank you as well for your patience and acceptance of the change. It was a pleasure to work on this.

Von: lessonz [mailto:notifications@github.com]
Gesendet: Donnerstag, 12. Dezember 2013 05:27
An: jenkinsci/fitnesse-plugin
Cc: Rico Gaudlitz
Betreff: Re: [fitnesse-plugin] changes to overcome performance issues with huge fitnesse result files (#10)

Thanks so much for working through the issues. It's greatly appreciated.


Reply to this email directly or view it on GitHubhttps://github.com//pull/10#issuecomment-30388600.

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.