Hi David,
Thank you for your comment.
Is there some reason the option string could not simply become part of the
existing configuration string?
My first proposal keeps option string at LogOutput and its child class (See
webrev.01).
Marcus commented that option string should be generated from current
configuration.
I uploaded new webrev.
Could you review again?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.04/
Thanks,
Yasumasa
On 2016/06/12 6:44, David Holmes wrote:
Hi Yasumasa,
Sorry but this API seems poorly fitting to me. First print_option_string seems the wrong name given
that the base class, LogOutput, has no notion of having an "option string". It seems to
be a supposedly generic "print other stuff" function that only one class actually needs
to implement.
Secondly it inverts the style of the API used for everything else - we have getters for all the
other "properties" which are then printed by the describe_current_configuration method.
But this is instead a "print" function where we ask the target to print itself. Mixing
the two styles seems messy. It probably would have been better to have had a print-style API from
the start - then adding the options would have been a trivial extension for those output classes
with options.
In addition the change you made to describe_current_configuration is not at all general
purpose - you wanted a given format (print between the config string and the decorators)
for this one class and so you added the code to support that format. But that format may
not make sense for other classes that might have "extra stuff" to print.
Is there some reason the option string could not simply become part of the existing
configuration string? It seems to me that for a LogFile these "options" really
are part of the configuration.
Thanks,
David
PS. The two hpp files would need their copyright years updated to "2015, 2016,".
On 11/06/2016 10:30 PM, Yasumasa Suenaga wrote:
PING: Could you review it?
We need a second reviewer.
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.03/
This change is small fix, and it helps us to confirm current
FileLogOutput configuration.
So I want to merge it to jdk 9.
Thanks,
Yasumasa
On 2016/05/17 19:17, Yasumasa Suenaga wrote:
PING: Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.03/
Thanks,
Yasumasa
On 2016/05/10 8:06, Yasumasa Suenaga wrote:
We need a second reviewer.
Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.03/
Yasumasa
On 2016/05/04 23:38, Marcus Larsson wrote:
Hi,
On 05/04/2016 04:12 PM, Yasumasa Suenaga wrote:
Hi Marcus,
93 out->print("filecount=%u,filesize=" SIZE_FORMAT "%s ",
_file_count, byte_size_in_proper_unit(_rotate_size),
proper_unit_for_byte_size(_rotate_size));
Thanks, I applied it to new webrev:
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.03/
Looks OK.
Thanks,
Marcus
Could you review again?
Yasumasa
On 2016/05/04 22:35, Marcus Larsson wrote:
Hi,
On 05/04/2016 02:59 PM, Yasumasa Suenaga wrote:
Hi Marcus,
Thank you for your comment.
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.02/
Looks better. The format for _rotate_size should be SIZE_FORMAT.
While we're at it I think it would be good (as I mentioned) to use
a proper unit for the filesize. Basically changing
93 out->print("filecount=%u,filesize=%lu ", _file_count,
_rotate_size);
into
93 out->print("filecount=%u,filesize=" SIZE_FORMAT "%s ",
_file_count, byte_size_in_proper_unit(_rotate_size),
proper_unit_for_byte_size(_rotate_size));
Thanks,
Marcus
I fixed to use _rotate_size and _file_count directly to show
VM.log list jcmd.
I do not store option string, and I added new function to print
option string.
Could you review it again?
Thanks.
Yasumasa
On 2016/05/04 18:33, Marcus Larsson wrote:
Hi,
On 05/03/2016 01:43 PM, Yasumasa Suenaga wrote:
PING: Could you review and sponsor it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.01/
I would prefer to generate the option string from the actual
options rather than saving the string from when it was
configured. This would also produce/print the options for
outputs that are using the defaults (which is not the case now).
The filesize option could then use byte_size_in_proper_unit and
proper_unit_for_byte_size to make it easier to read.
Also, get_option_string() should just be called option_string().
Thanks,
Marcus
This patch makes to show option string of LogFileOutput.
Thanks,
Yasumasa
On 2016/04/19 22:55, Yasumasa Suenaga wrote:
I adapted changes to jdk9/hs/hotspot repos.
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.01/
Please review.
Yasumasa
On 2016/04/18 23:09, Yasumasa Suenaga wrote:
PING:
I've sent review request for JDK-8153074.
Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.00/
If this patch is merged, user can confirm output option via
VM.log jcmd.
Please review and sponsor it.
Thanks,
Yasumasa
On 2016/04/11 18:29, Yasumasa Suenaga wrote:
PING: Could you review and sponsor it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.00/
Thanks,
Yasumasa
On 2016/03/31 22:35, Yasumasa Suenaga wrote:
CC'ed to serviceability-dev.
Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.00/
Thanks,
Yasumasa
On 2016/03/30 23:09, Yasumasa Suenaga wrote:
Hi all,
This request review is related to [1].
I want to see output option (filecount, filesize) in
VM.log jcmd.
Output sample:
#2: gc.log gc=trace, filecount=5,filesize=1048576
time,level,
I uploaded webrev. Could you review it?
http://cr.openjdk.java.net/~ysuenaga/JDK-8153074/webrev.00/
I cannot access JPRT. So I need a sponsor.
Thanks,
Yasumasa
[1]
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2016-March/018704.html
