Support GC log file rotation about gcviewer

[chewiebug]

Hi Haim,

That looks like an interesting option - I'll have to look into it.

Regards, Jörg

[lifey]

Hi Jörg , really sorry
I forgot I opened this issue and I have opened the new one as a preliminary action before I start solving this issue

[chewiebug]

Hi Haim,

No problem. It is a very good idea to open an issue as a preliminary action before solving it!

I am looking forward to seeing your pull request!

Regards, Jörg

[geld0r]

Any progress on this?
I'd also be interested in this feature and could - depending on implementation effort - also try to work on this.

[chewiebug]

It looks, like Haim never got round to implement this issue. If you have some time to work on this one, I'll happily assign it to you. Would you like to have some hints on where to start?

By the way: Please pull the latest commits from the "develop" branch. I have just merged a feature, that included quite some refactoring to GCViewer.

[geld0r]

Yes, hints are very welcome. I'll make sure to pull the develop-branch.

What I have been thinking about: How should non-consecutive logfiles be handled?
In contrast to log4j/logbacks rolling mechanism, the logfiles simply start with 0 after a JVM restart. In such a case the files from a previous run still remain, but must not be appended.

Edit: I already looked into this.
For actually merging the files I'd use IOUtils (from apache-commons-io). Its not a dependency of the project (in fact nothing from apache-commons is). Is that on purpose?

[chewiebug]

I think, I'd start here:

• extend GCResource with a flag, to say, if rotated log files should be
  loaded
• extend DataReaderFacade.loadModel() to load the whole list of the
  files and return them inside the same GCModel object

To tackle your question concerning non-consecutive logfiles: The class
GCModel has an inner class "FileInformation" that is able to hold size +
lastmodified timestamp of a file or an url. Maybe, we can use this class
to check, if a next file in the sequence is younger, than the current
and only then load the next file. If it is not younger, stop loading.

Probably, the DataReader interface should be extended, so that an
existing GCModel can be passed in. So that with those consecutive files
loaded, all events can be put into the same GCModel object.

Maybe, as a second step, a menu item could be added to be able to turn
loading of the consecutive files on and off. If we even extend
GCPreferences, we can also save the item in the properties file.

To complete the whole thing, a command line parameter could be added to
allow to choose the behaviour in that respect as well.

What do you think? Can you start with this information? If you would
like to have more hints / discuss ideas, please just tell. If you can
implement the first step, I'd say the main part of the solution is
there. If you still have time, you may attack more steps.

[geld0r]

If already started in the meantime and followed the apporach that I use when doing this by hand:

• Add new menu option to load several files as a series
• files are (attempted to be) sorted ascendingly. If possible this is done by timestamps in the log (e.g. when either -XX:+PrintGCTimeStamps or -XX:+PrintGCDateStamps was active). As a fallback this could also sort by file by creation date as you suggested.
• The file is then merged and then loaded as if it was a single file.

What do you think of this approach?
Your other suggestions can of course be integrated, e.g. when the GcModel was already built for sorting it could of course be reused for building the merged model/file.
Adding a commandline option sounds nice as well.

[chewiebug]

I like the idea to have a new menu option for the new feature. Like this, it is very clear for the user, if he uses the new / different functionality or still the one he was used to. Your approach with first merging the files and then parsing them will also work. As long as you keep the logic of how to find out, which files to merge and parse under control of the DataReaderFacade, extending the implementation for usage with commandline will be very easy. Implementing the logic inside DataReaderFacade will probably proove to be more difficult, because more classes need to be changed. If you prefer to implement a more straightforward approach, just go ahead. Refactoring it later should not be overly difficult. By the way: The current implementation of GCModel relies on the parser to add the events sorted by timestamp. It contains no sorting logic of its own.

[geld0r]

You can have a look at a current WIP state here:
https://github.com/geld0r/GCViewer/tree/feature/%2361

As far as my testing goes this works fine. I'll start with the refactoring you mentioned next.

[chewiebug]

Looks good! I very much like your unittesting! The FileMerger class, especially #getFirstTimeStamp() is yet inefficient, but you obviously know that. I am looking forward to your refactoring. Would it help your refactoring, if there was a method DataReader#read(GCModel model) that would append to an existing GCModel?

[geld0r]

Check the latest state (incl. some refactoring) here:
https://github.com/geld0r/GCViewer/tree/feature/%2361
(is this the right way to reference this?).

I hope I have tackled the issues you mentioned. I've done some refactoring as you suggested. Hopefully I didn't go overboard.

Open points from my side:

• Separate menu icon for loading a series of logfiles - where did you get the existing ones from?
• When loading several logfiles a warning could be shown when they overlap (meaning: not all files belong to one series) I gave this another thought and I think this may not be required.
• The "Parser" tab doesn't show the logger content - I couldn't find out why yet. Also the progress bar doesn't work when loading a series (related?)
• "Open recent files" doesn't work when the file were opened as a series (they are opened separately instead)
• Commandline stuff still missing. Need some pointers here on how this should be implemented. Specifically: Whats the desired format? I went ahead with a possible implementation
• Fallback when timestamps are missing in logfile is not yet implemented
• Refresh/Reload doesn't work yet for series. Could be straight forward "full" reload. Alternativly could also just reload the last file in the series. Same goes for the "monitor" option.

I'll address those points next. Let me know if you have feedback.

[geld0r]

Did you get a chance to look into this already?

[chewiebug]

Unfortunately not. I am currently very busy. I might have time to take a very short look until end of this week. I'll let you know what comes out.

-------- Original Message --------
From: geld0r [email protected]
Sent: 22 May 2016 19:31:16 CEST
To: chewiebug/GCViewer [email protected]
Cc: chewiebug [email protected], Comment [email protected]
Subject: Re: [chewiebug/GCViewer] Support GC log file rotation (#61)

Did you get a chance to look into this already?

---

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#61 (comment)

[chewiebug]

Ok, I have now taken a look. Since I am currently on vacation, I could only check the diffs and the code online, which makes the review a bit more difficult.

From what I see there, it looks good! Your approach is quite flexible.

From checking the code, I have one small question: in GcSeriesLoader#getCreationDate() you log a warning, if there are no datestamps present in the logfile. Does model#getfirstDateStamp() infer a datestamp, if there are only timestamps? If so, the fallback would only be necessary, if there was neither datestamp nor timestamp present.

Some remarks to details, I have found:

• indent should always be 4 blanks (not 6)
• { should not be on their own line, but in the end of the last line.

I might find more after vacation, when I can test on my computer, but that will probably again take at least three weeks from now. In general, I'd say, it looks good. I'd be glad, if you could do the formatting cleanup and then you may open a pull request. Unless you would like to wait a few weeks until I'll have time to take a deeper look.

Thank you very much and best regards, Jörg

-------- Original Message --------
From: "Jörg Wüthrich" [email protected]
Sent: 22 May 2016 22:06:14 CEST
To: chewiebug/GCViewer [email protected]
Subject: Re: [chewiebug/GCViewer] Support GC log file rotation (#61)

Unfortunately not. I am currently very busy. I might have time to take a very short look until end of this week. I'll let you know what comes out.

-------- Original Message --------
From: geld0r [email protected]
Sent: 22 May 2016 19:31:16 CEST
To: chewiebug/GCViewer [email protected]
Cc: chewiebug [email protected], Comment [email protected]
Subject: Re: [chewiebug/GCViewer] Support GC log file rotation (#61)

Did you get a chance to look into this already?

---

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#61 (comment)

[geld0r]

Thanks for having having a look before your vacation!

I've just finished the code style changes. I didn't use 6 blanks, but tabs instead of spaces. This wasn't consistent in all places before. I tried to only align this in the places I've touched, but I may have also aligned some lines surrounding that.
For consistency, it would be good if you could document this (contributing.md) or ideally also provide a code-style template (in Eclipse format, IntelliJ Idea also understands this).

GcModel#getfirstDateStamp() doesn't infer a datestamp if ony a timestamp exists. You are right, I'll also add a check for the timestamp.

The only thing that remains open from my side is adding a seperate icon for "load series".
I am fine with waiting a few more weeks in order to give you some time for a proper review. No need to rush this in.

Enjoy your vacation! I am looking forward to your review after that.

Edit: As a bonus I've also improved the parsing times of logs. It doesn't strictly belong to this issue, but avoids rebasing the change. If you don't want this as part of the same pull request, let me know.

[chewiebug]

Ok, I 'd also prefer to have some more time for a proper review, where I can also test the implementation. I hope to be able to do it within two or three weeks. Good hint concerning code style template! It will help to avoid these small inconsistencies that make reviewing code more difficult. I think, there is currently some implementation in the gui (the code that switches the timeline between "time between start" and "absolute date"), which sits exactly this inference of the start date based on the file date, if there is no datestamp in the file. That code had probably better be moved into gcmodel to accommodate your usecase. Best regards, Jörg

[chewiebug]

Hmm, unfortunately, I still didn't find the time to do the proper review. A passing glance tells me, that your latest commit breaks the deployment mechanism, I built with Maven. Since you added a library to the dependencies, there is now the need for an assembly for the distribution, which includes that library. I assume, that the same mechanism can be used, as is in place for the mac distribution (see src/man/assembly). There needs one additional step - download the library. Do you know Maven enough, that you could also fix this? Concerning the review, I don't think, I'll have time before end of July, I am afraid!

[chewiebug]

Ok, finally, I have had time to check your branch. I must say, I am still very impressed of you work - I especially like, how many unittests you write to test your code. You have also found a way around the 2 times loading of all files - very good!

I agree, that the feature is almost finished with implementation! I have only minor things to remark. Please tell me, if you would like to work on them, or if you leave it to me. In that case, I'd add the few remaining bits and merge it to develop, after.

I have found just one thing, that doesn't work as expected: using the commandline mode with a series of files results in an UnsupportedOperationException because the GCResource passed to DataReaderFacade#loadModel() is a GCResourceSeries. I believe, you could allow for that inside DataReaderFacade and decide there, which mechanism for loading to use.

One cosmetic detail: The display string of a series in the recent menu is a bit long. I think, I'd prefer to have something like " (series, 6 more files)" or something similar.

That's already all I have found.

Concerning the icon: I'll try to find something.

Concerning the bonus: Thank you - and yes, I would like to have it in a separate pull request, because it will need some work in the build process.

Thanks again!

Best regards,
Jörg

[geld0r]

Thanks for having a thorough look!
Regarding your remarks: I think I could handle that. Unless it would take longer for you to explain it to me compared to you doing it yourself. The last time I checked there was also one UnitTest that was broken... probably due to some error while rebasing. I will have a look at that as well as the two things that you mentioned.

You are right that the build is broken with the added dependency. I've tried already to get this to work but failed. Apparently it is nowadays suggested to use the maven-shade plugin to built a fat-jar with dependencies for apps like this. I could try this out and will prepare a second pull request for that.

[chewiebug]

@geld0r: I have merged your pull request #176.

I'll close this issue. You may start with the performance improvement pull request at your convenience :-).

By the way: Your proposal to use the maven-shade plugin looks good to me.

[geld0r]

Thank you!

I've added PR #177 which contains the remaining performance improvement & build adjustments.
I've checked myself that the resulting jar now works and that the dependency is included. As I am not familiar with how GCViewer ist distributed, please make sure that now the jar that includes dependencies is chosen (that however should automatically be the case since the file name didn't change).

[chewiebug]

Hi

Thank you very much for your second pull request! I have reviewed it, implementation looks ok.

Concerning distribution, I'll have to draw a diagram to visualise, what the result should be. Unfortunately, I think, the deistribution process is a little complicated.

I'll post it as a comment to pull request #177.

Best regards,
Jörg