On Wed, Aug 14, 2019 at 9:17 AM Severin Gehwolf <sgehwolf at redhat.com> wrote:
>
> Hi Jie,
>
> On Mon, 2019-08-12 at 14:58 -0400, Jie Kang wrote:
> > Hi all,
> >
> > Please review this jdk11u backport. The patch did *not* apply cleanly
> > and has changes described following the links below. A CSR was opened
> > and approved for this backport. As well Oracle has brought this for
> > 11.0.6 so we should not push this until 11.0.6 opens (Aug 27).
> >
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8205516
> > Webrev: http://cr.openjdk.java.net/~jkang/8205516/webrev.01/
>
> src/jdk.jfr/share/classes/jdk/jfr/internal/Utils.java
>
> JDK 11u has JDK-8215175 already applied which removes this function:
> public static String formatBytes(long bytes, String separation)
>
> Thus, changes to Utils.java don't apply for JDK 11u. OK.
>
> src/jdk.jfr/share/classes/jdk/jfr/internal/tool/PrettyWriter.java
> src/jdk.jfr/share/classes/jdk/jfr/consumer/RecordedObject.java
>
> Changes from JDK-8165675 incorporated as you say. OK
>
> src/jdk.jfr/share/classes/jdk/jfr/internal/tool/PrettyWriter.java
>
> You seem to have introduced an extra newline at line 314. Please remove
> as this might cause issues on further backports.
Hi Severin,
Thank you for the review and catching this. I have an updated webrev
with the newline removed:
http://cr.openjdk.java.net/~jkang/8205516/webrev.02/
Regards,
>
> 517 String bytes = Utils.formatBytes(n.longValue());
>
> Changed 'Utils.formatBytes(n.longValue(), " ")' to
> 'Utils.formatBytes(n.longValue())' so as to account for JDK-8215175
> already been present in JDK 11u as you said. OK.
>
> test/jdk/jdk/jfr/api/metadata/annotations/TestFormatMissingValue.java
>
> Test adjusted back to post JDK-8215175 changes in JDK 11u. OK.
>
> Thanks,
> Severin
>
>
> > CSR: https://bugs.openjdk.java.net/browse/JDK-8228670
> >
> > * Addition of changes to PrettyWriter and RecordedObject from JDK-8165675 [1]
> >
> > PrettyWriter is newly added with this backport. Since it did not exist
> > at the time for the JDK-8165675 backport, the changes there for
> > PrettyWriter were not included. These are included within this patch.
> > As well some lines in the JDK-8165675 backport were skipped for
> > RecordedObject causing a test to fail, and are now included so the
> > test passes. Let me know if there is a different process for this
> > situation!
> >
> > [1]
> > https://bugs.openjdk.java.net/browse/JDK-8165675
> > PrettyWriter changes:
> > http://hg.openjdk.java.net/jdk/jdk/rev/4eff16f47ae2#l5.2
> > RecordedObject changes:
> > http://hg.openjdk.java.net/jdk/jdk/rev/4eff16f47ae2#l4.35
> >
> > * Updates to test/jdk/jdk/jfr/api/metadata/annotations/TestFormatMissingValue.java
> >
> > This test is updated to match this backport. There are comments in the
> > changeset visible that describe why.
> >
> > * A small update to
> > src/jdk.jfr/share/classes/jdk/jfr/internal/tool/PrettyWriter.java
> >
> > if (dataAmount != null) {
> > if (value instanceof Number) {
> > Number n = (Number) value;
> > - String bytes = Utils.formatBytes(n.longValue(), " ");
> > + String bytes = Utils.formatBytes(n.longValue());
> > if (field.getAnnotation(Frequency.class) != null) {
> > bytes += "/s";
> > }
> >
> > This is to match the function definition of Utils.formatBytes in
> > jdk11u-dev. This is actually also what is in jdk/jdk, but it is a
> > change here as the order of commits in jdk11u-dev differ from jdk/jdk.
> >
> >
> > Also. this backport should be integrated with the two follow-up bug
> > fixes at the same time to maintain a clean working repository
> > (compiles and tests pass). Both cleanly apply after my patch. I have
> > added fix request label and comment to them.
> >
> > JDK-8214896: JFR Tool left files behind
> > https://bugs.openjdk.java.net/browse/JDK-8214896
> >
> > JDK-8214925: [TESTBUG] JFR tool fails to execute
> > https://bugs.openjdk.java.net/browse/JDK-8214925
> >
> > I have run the tier one tests, jdk.jfr tests and some manual tests of
> > the jfr tool binary with all three fixes applied together, but third
> > party verification would be greatly appreciated. Let me know what you
> > think!
> >
> >
> > Regards,
>