[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: JDK-8199729 - Usage Logger open sourcing
From: Alan Bateman <Alan.Bateman () oracle ! com>
Date: 2018-05-31 12:42:27
Message-ID: bf532c84-ba5a-e0a0-8db2-c2b26d88e903 () oracle ! com
[Download RAW message or body]
On 30/05/2018 16:50, Sharath Ballal wrote:
>
> Hello,
>
> Requesting reviews for Usage Logger open sourcing (earlier known as
> Usage Tracker).
>
> ID: https://bugs.openjdk.java.net/browse/JDK-8199729
>
> Webrev: http://cr.openjdk.java.net/~sballal/8199729/webrev.00/
> <http://cr.openjdk.java.net/%7Esballal/8199729/webrev.00/>
>
>
I skimmed through this. I think the implementation will need significant
clean-up before it is ready for code review. Also it feels like this
feature needs more a lot context to understand how the data is consumed
and whether the log files and UDP messages are intended to be a
supported interface or not.
The use of system wide locations for configuration files is something
that needs discussion as it's not clear (to me anyway) if the locations
chosen are appropriate in all cases.
In terms of code organization then I assume the code to check for
usagelogger.properties should not be in PostVMInitHook, it should be
moved to UserLoggerClient. Also there seems to be a dependency on
EnvUtils that is not in the webrev. The package name for this code will
also need discussion, I assume it needs to be moved to jdk.internal.
-Alan
[Attachment #3 (text/html)]
<html>
<head>
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
</head>
<body text="#000000" bgcolor="#FFFFFF">
On 30/05/2018 16:50, Sharath Ballal wrote:<br>
<blockquote type="cite"
cite="mid:36b5fda6-85d8-450a-8344-0050642ee540@default">
<meta http-equiv="Content-Type" content="text/html;
charset=windows-1252">
<meta name="Generator" content="Microsoft Word 15 (filtered
medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri","sans-serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:#0563C1;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:#954F72;
text-decoration:underline;}
p.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0in;
margin-right:0in;
margin-bottom:0in;
margin-left:.5in;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri","sans-serif";}
span.EmailStyle17
{mso-style-type:personal-compose;
font-family:"Calibri","sans-serif";
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
/* List Definitions */
@list l0
{mso-list-id:848174104;
mso-list-type:hybrid;
mso-list-template-ids:-233912786 67698703 67698713 67698715 67698703 67698713 \
67698715 67698703 67698713 67698715;} @list l0:level1
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level2
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level3
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l0:level4
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level5
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level6
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
@list l0:level7
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level8
{mso-level-number-format:alpha-lower;
mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-.25in;}
@list l0:level9
{mso-level-number-format:roman-lower;
mso-level-tab-stop:none;
mso-level-number-position:right;
text-indent:-9.0pt;}
ol
{margin-bottom:0in;}
ul
{margin-bottom:0in;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
<div class="WordSection1">
<p class="MsoNormal">Hello,<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Requesting reviews for Usage Logger open
sourcing (earlier known as Usage Tracker).<o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">ID: <a
href="https://bugs.openjdk.java.net/browse/JDK-8199729"
moz-do-not-send="true">https://bugs.openjdk.java.net/browse/JDK-8199729</a>
<o:p></o:p></p>
<p class="MsoNormal">Webrev: <a
href="http://cr.openjdk.java.net/%7Esballal/8199729/webrev.00/"
moz-do-not-send="true">http://cr.openjdk.java.net/~sballal/8199729/webrev.00/</a>
<o:p></o:p></p>
<br>
</div>
</blockquote>
I skimmed through this. I think the implementation will need
significant clean-up before it is ready for code review. Also it
feels like this feature needs more a lot context to understand how
the data is consumed and whether the log files and UDP messages are
intended to be a supported interface or not.<br>
<br>
The use of system wide locations for configuration files is
something that needs discussion as it's not clear (to me anyway) if
the locations chosen are appropriate in all cases.<br>
<br>
In terms of code organization then I assume the code to check for
usagelogger.properties should not be in PostVMInitHook, it should be
moved to UserLoggerClient. Also there seems to be a dependency on
EnvUtils that is not in the webrev. The package name for this code
will also need discussion, I assume it needs to be moved to
jdk.internal.<br>
<br>
-Alan<br>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic