Review request for JM-6544: Add support to parse Java internal version of Zulu JVM

[Henrik Dafgård]

Hi Florian,

This change looks good to me!


Cheers,
Henrik Dafgård


On Tue, 6 Aug 2019 at 11:12, Florian David <florian.david at datadoghq.com>
wrote:

> Thanks for your feedback, I understand the pain of having to maintain
> different vendors version in the upstream project. As Henrik wrote, having
> a long-term solution for this would involve a longer discussion.
>
> Meanwhile, we can still fix this issue with the solution mentioned above
> and having a correct error message reported to the user, providing a better
> understanding than just a NPE.
>
> Here is the updated patch:
> JIRA: https://bugs.openjdk.java.net/browse/JMC-6544
> Webrev: http://cr.openjdk.java.net/~fdavid/JMC-6544/webrev.1/
>
> Thanks,
> Florian
>
>
>
> On Mon, Aug 5, 2019 at 6:44 PM Henrik Dafgård <hdafgard at gmail.com> wrote:
>
>> Hi all,
>>
>> I think that this is a more involved discussion that should definitely be
>> had. Originally we only built JMC to support known JDKs built by Oracle
>> where we could be assumed to know enough details about the features of
>> each
>> version to depend on only version numbers to decide whether to check for
>> certain behaviours in the rules. This is in e.g. the String Deduplication
>> rule and the Code Cache rule, they're already depending on vendor, albeit
>> implicitly.
>>
>> To fix this properly we could perhaps build some support in the core JMC
>> APIs for checking Java Vendor and allowing downstream implementations to
>> react to different vendors, rather than just to which JDK version is used.
>> We would then have to replace all references to
>> RulesToolkit.getJavaVersion
>> used for determining runtime features with both checks for vendor and
>> version.
>>
>> Regardless, we should still fix this issue and after looking through
>> usages
>> of the JavaVersion methods I've found that most of them check for a
>> potential null return value and only two don't (and they both have
>> implicit
>> vendor dependencies). So the best fix is probably to add null checks to
>> the
>> FlightRecordingSupportRule:113 with a N/A result and the
>> CodeCachePageUI:294.
>>
>>
>> Cheers,
>> Henrik Dafgård
>>
>>
>> On Mon, 5 Aug 2019 at 10:46, Mario Torre <neugens at redhat.com> wrote:
>>
>> > On 05/08/2019 10:31, Florian David wrote:
>> > > I have been told that the patch was removed from the previous mail.
>> > > Please find a web review instead:
>> > >
>> > > JIRA: https://bugs.openjdk.java.net/browse/JMC-6544
>> > > Webrev: http://cr.openjdk.java.net/~fdavid/JMC-6544/webrev.0/
>> >
>> > Hi Florian,
>> >
>> > To be honest, I already expressed concerns about this
>> > FlightRecordingSupportRule, I don't think it's a good idea to make JMC
>> > know every single java vendor out there in the world.
>> >
>> > I would rather prefer such patches are either internal to vendors and
>> > not exposed in the upstream project, but more generally we should rework
>> > this mechanisms in order to be vendor-agnostic rather than add more
>> > special cases.
>> >
>> > Cheers,
>> > Mario
>> >
>> > > On Sun, Aug 4, 2019 at 6:12 PM Florian David <
>> > florian.david at datadoghq.com>
>> > > wrote:
>> > >
>> > >> Hi,
>> > >>
>> > >>
>> > >> Please find attached a patch to solve an error that arise when
>> parsing
>> > >> Zulu internal version [0]. I also added a test with a representative
>> > set of
>> > >> Java internal versions of the Zulu JVM, even if it's far from
>> covering
>> > >> every single case.
>> > >>
>> > >>
>> > >> Regards,
>> > >>
>> > >> Florian
>> > >>
>> > >>
>> > >> [0] https://bugs.openjdk.java.net/browse/JMC-6544
>> > >>
>> > >>
>> > >>
>> > >>
>> >
>> >
>> > --
>> > Mario Torre
>> > Associate Manager, Software Engineering
>> > Red Hat GmbH <https://www.redhat.com>
>> > 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898
>> >
>>
>