You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Owen Nichols <on...@pivotal.io> on 2019/05/16 01:09:24 UTC
[DISCUSS] reduce PR checks to JDK11 only
Currently every PR commit triggers both JDK8 and JDK11 versions of each test job. I propose that we can eliminate the JDK8 version of each check. In the extremely rare case where a code change breaks on Java 8 but works fine on Java 11, it would still be caught by the main pipeline (just as Windows failures are caught only in the main pipeline).
The only tangible effect today of running both JDK8 and JDK11 tests in PR pipeline is twice the chance to encounter possible flaky failures (usually unrelated to the commit itself).
Re: [DISCUSS] reduce PR checks to JDK11 only
Posted by Darrel Schneider <ds...@pivotal.io>.
I think it would be a mistake to have just the main pipeline catch
analyze-serializable failures. It is easy to accidentally cause these
failures so I think we want them caught early. They are also not flaky so
if broken then they will consistently fail. I think this would cause
someone extra work in tracking down the change, backing it out, and
resubmitting the PR once fixed.
I would vote for getting these checks back into PR checks as quickly as
possible. That is probably "revert PR 3598". Then someone who knows these
tests well can propose the best way to improve them. But after reverting
you could also resubmit it with something like #2 or #3 that runs the
existing tests that require JDK 8 but not all the other tests.
On Sun, May 19, 2019 at 2:20 AM Owen Nichols <on...@pivotal.io> wrote:
> Correct, analyze-serializables test is currently skipped under JDK11.
> Ideally it would be re-written to test what is actually serialized, not the
> flawed assumption that no change in compiled bytecode size means nothing
> changed...
>
> This also brings up a good question worth discussing: is the goal of PR
> checks to catch all of the same issues as the main pipeline, or is it ever
> ok to run some tests only in the main pipeline (for example, PR checks have
> never included Windows tests).
>
> So which is the preferred course of action?
> 1) restore ALL JDK8 tests to PR checks (i.e. revert PR 3598)
> 2) restore just the one JDK8 test job that includes analyze-serializables
> to the PR checks
> 3) create a tailored subset of JDK8-specific tests to include in PR checks
> 4) just let the main pipeline catch analyze-serializables failures?
> 5) bite the bullet and fix analyze-serializables to test serialization in
> a more meaningful way that works on both JDK8 and 11
>
> -Owen
>
> > On May 17, 2019, at 1:21 PM, Darrel Schneider <ds...@pivotal.io>
> wrote:
> >
> > One problem with doing this, I think, is that we have some tests that are
> > disabled unless run on 8. They are the analyze serializables tests. I'm
> not
> > sure of the details but I think the "golden" checksums these tests
> compare
> > to were generated from the byte codes from the jdk 8 class files. The
> byte
> > codes are different for jdk 11. So by pull requests runs only happening
> on
> > jdk 11 we will lose coverage. These tests catch if changed the
> serializable
> > format of classes. I think if the "golden" checksums were regenerated
> with
> > jdk 11 then these tests could be enabled when run on jdk 11. Others on
> the
> > dev list may have more of the details.
> >
> > On Thu, May 16, 2019 at 5:31 PM Owen Nichols <on...@pivotal.io>
> wrote:
> >
> >> I’ve created a PR for this, please give it a +1:
> >> https://github.com/apache/geode/pull/3598
> >>
> >>> On May 16, 2019, at 11:12 AM, Anilkumar Gingade <ag...@pivotal.io>
> >> wrote:
> >>>
> >>> Make sense to me...Looking at the probability of breaking specific to,
> >> jdk8
> >>> and jdk11 through a commit.
> >>>
> >>>
> >>> On Wed, May 15, 2019 at 6:09 PM Owen Nichols <on...@pivotal.io>
> >> wrote:
> >>>
> >>>> Currently every PR commit triggers both JDK8 and JDK11 versions of
> each
> >>>> test job. I propose that we can eliminate the JDK8 version of each
> >> check.
> >>>> In the extremely rare case where a code change breaks on Java 8 but
> >> works
> >>>> fine on Java 11, it would still be caught by the main pipeline (just
> as
> >>>> Windows failures are caught only in the main pipeline).
> >>>>
> >>>> The only tangible effect today of running both JDK8 and JDK11 tests in
> >> PR
> >>>> pipeline is twice the chance to encounter possible flaky failures
> >> (usually
> >>>> unrelated to the commit itself).
> >>>>
> >>>>
> >>>>
> >>
> >>
>
>
Re: [DISCUSS] reduce PR checks to JDK11 only
Posted by Owen Nichols <on...@pivotal.io>.
@rhoughton - sounds reasonable. JDK8 Unit tests are added back to PR checks now.
@jbarrett - good point, no need to skip analyze-serializables test when compiled with JDK8. Jinmei has a PR to un-skip: https://github.com/apache/geode/pull/3607
@dschneider - do you still want to bring back all JDK8 PR checks for now, or would merging Jinmei’s fix sufficiently address your concerns?
> On May 20, 2019, at 8:56 AM, Jacob Barrett <jb...@pivotal.io> wrote:
>
> I that the serialization test was only an issue when compiled with JDK11? We don’t compile with JDK11, we compile with JDK8 and run under JDK11.
>
>> On May 20, 2019, at 8:50 AM, Robert Houghton <rh...@pivotal.io> wrote:
>>
>> The PR pipeline should be a timely test, but also sane and helpful. Maybe
>> making the JDK8 Unit tests (but not the integration, etc) part of PR is a
>> good compromise in that sense.
>>
>>> On Sun, May 19, 2019 at 2:20 AM Owen Nichols <on...@pivotal.io> wrote:
>>>
>>> Correct, analyze-serializables test is currently skipped under JDK11.
>>> Ideally it would be re-written to test what is actually serialized, not the
>>> flawed assumption that no change in compiled bytecode size means nothing
>>> changed...
>>>
>>> This also brings up a good question worth discussing: is the goal of PR
>>> checks to catch all of the same issues as the main pipeline, or is it ever
>>> ok to run some tests only in the main pipeline (for example, PR checks have
>>> never included Windows tests).
>>>
>>> So which is the preferred course of action?
>>> 1) restore ALL JDK8 tests to PR checks (i.e. revert PR 3598)
>>> 2) restore just the one JDK8 test job that includes analyze-serializables
>>> to the PR checks
>>> 3) create a tailored subset of JDK8-specific tests to include in PR checks
>>> 4) just let the main pipeline catch analyze-serializables failures?
>>> 5) bite the bullet and fix analyze-serializables to test serialization in
>>> a more meaningful way that works on both JDK8 and 11
>>>
>>> -Owen
>>>
>>>> On May 17, 2019, at 1:21 PM, Darrel Schneider <ds...@pivotal.io>
>>> wrote:
>>>>
>>>> One problem with doing this, I think, is that we have some tests that are
>>>> disabled unless run on 8. They are the analyze serializables tests. I'm
>>> not
>>>> sure of the details but I think the "golden" checksums these tests
>>> compare
>>>> to were generated from the byte codes from the jdk 8 class files. The
>>> byte
>>>> codes are different for jdk 11. So by pull requests runs only happening
>>> on
>>>> jdk 11 we will lose coverage. These tests catch if changed the
>>> serializable
>>>> format of classes. I think if the "golden" checksums were regenerated
>>> with
>>>> jdk 11 then these tests could be enabled when run on jdk 11. Others on
>>> the
>>>> dev list may have more of the details.
>>>>
>>>> On Thu, May 16, 2019 at 5:31 PM Owen Nichols <on...@pivotal.io>
>>> wrote:
>>>>
>>>>> I’ve created a PR for this, please give it a +1:
>>>>> https://github.com/apache/geode/pull/3598
>>>>>
>>>>>> On May 16, 2019, at 11:12 AM, Anilkumar Gingade <ag...@pivotal.io>
>>>>> wrote:
>>>>>>
>>>>>> Make sense to me...Looking at the probability of breaking specific to,
>>>>> jdk8
>>>>>> and jdk11 through a commit.
>>>>>>
>>>>>>
>>>>>> On Wed, May 15, 2019 at 6:09 PM Owen Nichols <on...@pivotal.io>
>>>>> wrote:
>>>>>>
>>>>>>> Currently every PR commit triggers both JDK8 and JDK11 versions of
>>> each
>>>>>>> test job. I propose that we can eliminate the JDK8 version of each
>>>>> check.
>>>>>>> In the extremely rare case where a code change breaks on Java 8 but
>>>>> works
>>>>>>> fine on Java 11, it would still be caught by the main pipeline (just
>>> as
>>>>>>> Windows failures are caught only in the main pipeline).
>>>>>>>
>>>>>>> The only tangible effect today of running both JDK8 and JDK11 tests in
>>>>> PR
>>>>>>> pipeline is twice the chance to encounter possible flaky failures
>>>>> (usually
>>>>>>> unrelated to the commit itself).
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
Re: [DISCUSS] reduce PR checks to JDK11 only
Posted by Jacob Barrett <jb...@pivotal.io>.
I that the serialization test was only an issue when compiled with JDK11? We don’t compile with JDK11, we compile with JDK8 and run under JDK11.
> On May 20, 2019, at 8:50 AM, Robert Houghton <rh...@pivotal.io> wrote:
>
> The PR pipeline should be a timely test, but also sane and helpful. Maybe
> making the JDK8 Unit tests (but not the integration, etc) part of PR is a
> good compromise in that sense.
>
>> On Sun, May 19, 2019 at 2:20 AM Owen Nichols <on...@pivotal.io> wrote:
>>
>> Correct, analyze-serializables test is currently skipped under JDK11.
>> Ideally it would be re-written to test what is actually serialized, not the
>> flawed assumption that no change in compiled bytecode size means nothing
>> changed...
>>
>> This also brings up a good question worth discussing: is the goal of PR
>> checks to catch all of the same issues as the main pipeline, or is it ever
>> ok to run some tests only in the main pipeline (for example, PR checks have
>> never included Windows tests).
>>
>> So which is the preferred course of action?
>> 1) restore ALL JDK8 tests to PR checks (i.e. revert PR 3598)
>> 2) restore just the one JDK8 test job that includes analyze-serializables
>> to the PR checks
>> 3) create a tailored subset of JDK8-specific tests to include in PR checks
>> 4) just let the main pipeline catch analyze-serializables failures?
>> 5) bite the bullet and fix analyze-serializables to test serialization in
>> a more meaningful way that works on both JDK8 and 11
>>
>> -Owen
>>
>>> On May 17, 2019, at 1:21 PM, Darrel Schneider <ds...@pivotal.io>
>> wrote:
>>>
>>> One problem with doing this, I think, is that we have some tests that are
>>> disabled unless run on 8. They are the analyze serializables tests. I'm
>> not
>>> sure of the details but I think the "golden" checksums these tests
>> compare
>>> to were generated from the byte codes from the jdk 8 class files. The
>> byte
>>> codes are different for jdk 11. So by pull requests runs only happening
>> on
>>> jdk 11 we will lose coverage. These tests catch if changed the
>> serializable
>>> format of classes. I think if the "golden" checksums were regenerated
>> with
>>> jdk 11 then these tests could be enabled when run on jdk 11. Others on
>> the
>>> dev list may have more of the details.
>>>
>>> On Thu, May 16, 2019 at 5:31 PM Owen Nichols <on...@pivotal.io>
>> wrote:
>>>
>>>> I’ve created a PR for this, please give it a +1:
>>>> https://github.com/apache/geode/pull/3598
>>>>
>>>>> On May 16, 2019, at 11:12 AM, Anilkumar Gingade <ag...@pivotal.io>
>>>> wrote:
>>>>>
>>>>> Make sense to me...Looking at the probability of breaking specific to,
>>>> jdk8
>>>>> and jdk11 through a commit.
>>>>>
>>>>>
>>>>> On Wed, May 15, 2019 at 6:09 PM Owen Nichols <on...@pivotal.io>
>>>> wrote:
>>>>>
>>>>>> Currently every PR commit triggers both JDK8 and JDK11 versions of
>> each
>>>>>> test job. I propose that we can eliminate the JDK8 version of each
>>>> check.
>>>>>> In the extremely rare case where a code change breaks on Java 8 but
>>>> works
>>>>>> fine on Java 11, it would still be caught by the main pipeline (just
>> as
>>>>>> Windows failures are caught only in the main pipeline).
>>>>>>
>>>>>> The only tangible effect today of running both JDK8 and JDK11 tests in
>>>> PR
>>>>>> pipeline is twice the chance to encounter possible flaky failures
>>>> (usually
>>>>>> unrelated to the commit itself).
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
Re: [DISCUSS] reduce PR checks to JDK11 only
Posted by Robert Houghton <rh...@pivotal.io>.
The PR pipeline should be a timely test, but also sane and helpful. Maybe
making the JDK8 Unit tests (but not the integration, etc) part of PR is a
good compromise in that sense.
On Sun, May 19, 2019 at 2:20 AM Owen Nichols <on...@pivotal.io> wrote:
> Correct, analyze-serializables test is currently skipped under JDK11.
> Ideally it would be re-written to test what is actually serialized, not the
> flawed assumption that no change in compiled bytecode size means nothing
> changed...
>
> This also brings up a good question worth discussing: is the goal of PR
> checks to catch all of the same issues as the main pipeline, or is it ever
> ok to run some tests only in the main pipeline (for example, PR checks have
> never included Windows tests).
>
> So which is the preferred course of action?
> 1) restore ALL JDK8 tests to PR checks (i.e. revert PR 3598)
> 2) restore just the one JDK8 test job that includes analyze-serializables
> to the PR checks
> 3) create a tailored subset of JDK8-specific tests to include in PR checks
> 4) just let the main pipeline catch analyze-serializables failures?
> 5) bite the bullet and fix analyze-serializables to test serialization in
> a more meaningful way that works on both JDK8 and 11
>
> -Owen
>
> > On May 17, 2019, at 1:21 PM, Darrel Schneider <ds...@pivotal.io>
> wrote:
> >
> > One problem with doing this, I think, is that we have some tests that are
> > disabled unless run on 8. They are the analyze serializables tests. I'm
> not
> > sure of the details but I think the "golden" checksums these tests
> compare
> > to were generated from the byte codes from the jdk 8 class files. The
> byte
> > codes are different for jdk 11. So by pull requests runs only happening
> on
> > jdk 11 we will lose coverage. These tests catch if changed the
> serializable
> > format of classes. I think if the "golden" checksums were regenerated
> with
> > jdk 11 then these tests could be enabled when run on jdk 11. Others on
> the
> > dev list may have more of the details.
> >
> > On Thu, May 16, 2019 at 5:31 PM Owen Nichols <on...@pivotal.io>
> wrote:
> >
> >> I’ve created a PR for this, please give it a +1:
> >> https://github.com/apache/geode/pull/3598
> >>
> >>> On May 16, 2019, at 11:12 AM, Anilkumar Gingade <ag...@pivotal.io>
> >> wrote:
> >>>
> >>> Make sense to me...Looking at the probability of breaking specific to,
> >> jdk8
> >>> and jdk11 through a commit.
> >>>
> >>>
> >>> On Wed, May 15, 2019 at 6:09 PM Owen Nichols <on...@pivotal.io>
> >> wrote:
> >>>
> >>>> Currently every PR commit triggers both JDK8 and JDK11 versions of
> each
> >>>> test job. I propose that we can eliminate the JDK8 version of each
> >> check.
> >>>> In the extremely rare case where a code change breaks on Java 8 but
> >> works
> >>>> fine on Java 11, it would still be caught by the main pipeline (just
> as
> >>>> Windows failures are caught only in the main pipeline).
> >>>>
> >>>> The only tangible effect today of running both JDK8 and JDK11 tests in
> >> PR
> >>>> pipeline is twice the chance to encounter possible flaky failures
> >> (usually
> >>>> unrelated to the commit itself).
> >>>>
> >>>>
> >>>>
> >>
> >>
>
>
Re: [DISCUSS] reduce PR checks to JDK11 only
Posted by Owen Nichols <on...@pivotal.io>.
Correct, analyze-serializables test is currently skipped under JDK11. Ideally it would be re-written to test what is actually serialized, not the flawed assumption that no change in compiled bytecode size means nothing changed...
This also brings up a good question worth discussing: is the goal of PR checks to catch all of the same issues as the main pipeline, or is it ever ok to run some tests only in the main pipeline (for example, PR checks have never included Windows tests).
So which is the preferred course of action?
1) restore ALL JDK8 tests to PR checks (i.e. revert PR 3598)
2) restore just the one JDK8 test job that includes analyze-serializables to the PR checks
3) create a tailored subset of JDK8-specific tests to include in PR checks
4) just let the main pipeline catch analyze-serializables failures?
5) bite the bullet and fix analyze-serializables to test serialization in a more meaningful way that works on both JDK8 and 11
-Owen
> On May 17, 2019, at 1:21 PM, Darrel Schneider <ds...@pivotal.io> wrote:
>
> One problem with doing this, I think, is that we have some tests that are
> disabled unless run on 8. They are the analyze serializables tests. I'm not
> sure of the details but I think the "golden" checksums these tests compare
> to were generated from the byte codes from the jdk 8 class files. The byte
> codes are different for jdk 11. So by pull requests runs only happening on
> jdk 11 we will lose coverage. These tests catch if changed the serializable
> format of classes. I think if the "golden" checksums were regenerated with
> jdk 11 then these tests could be enabled when run on jdk 11. Others on the
> dev list may have more of the details.
>
> On Thu, May 16, 2019 at 5:31 PM Owen Nichols <on...@pivotal.io> wrote:
>
>> I’ve created a PR for this, please give it a +1:
>> https://github.com/apache/geode/pull/3598
>>
>>> On May 16, 2019, at 11:12 AM, Anilkumar Gingade <ag...@pivotal.io>
>> wrote:
>>>
>>> Make sense to me...Looking at the probability of breaking specific to,
>> jdk8
>>> and jdk11 through a commit.
>>>
>>>
>>> On Wed, May 15, 2019 at 6:09 PM Owen Nichols <on...@pivotal.io>
>> wrote:
>>>
>>>> Currently every PR commit triggers both JDK8 and JDK11 versions of each
>>>> test job. I propose that we can eliminate the JDK8 version of each
>> check.
>>>> In the extremely rare case where a code change breaks on Java 8 but
>> works
>>>> fine on Java 11, it would still be caught by the main pipeline (just as
>>>> Windows failures are caught only in the main pipeline).
>>>>
>>>> The only tangible effect today of running both JDK8 and JDK11 tests in
>> PR
>>>> pipeline is twice the chance to encounter possible flaky failures
>> (usually
>>>> unrelated to the commit itself).
>>>>
>>>>
>>>>
>>
>>
Re: [DISCUSS] reduce PR checks to JDK11 only
Posted by Darrel Schneider <ds...@pivotal.io>.
One problem with doing this, I think, is that we have some tests that are
disabled unless run on 8. They are the analyze serializables tests. I'm not
sure of the details but I think the "golden" checksums these tests compare
to were generated from the byte codes from the jdk 8 class files. The byte
codes are different for jdk 11. So by pull requests runs only happening on
jdk 11 we will lose coverage. These tests catch if changed the serializable
format of classes. I think if the "golden" checksums were regenerated with
jdk 11 then these tests could be enabled when run on jdk 11. Others on the
dev list may have more of the details.
On Thu, May 16, 2019 at 5:31 PM Owen Nichols <on...@pivotal.io> wrote:
> I’ve created a PR for this, please give it a +1:
> https://github.com/apache/geode/pull/3598
>
> > On May 16, 2019, at 11:12 AM, Anilkumar Gingade <ag...@pivotal.io>
> wrote:
> >
> > Make sense to me...Looking at the probability of breaking specific to,
> jdk8
> > and jdk11 through a commit.
> >
> >
> > On Wed, May 15, 2019 at 6:09 PM Owen Nichols <on...@pivotal.io>
> wrote:
> >
> >> Currently every PR commit triggers both JDK8 and JDK11 versions of each
> >> test job. I propose that we can eliminate the JDK8 version of each
> check.
> >> In the extremely rare case where a code change breaks on Java 8 but
> works
> >> fine on Java 11, it would still be caught by the main pipeline (just as
> >> Windows failures are caught only in the main pipeline).
> >>
> >> The only tangible effect today of running both JDK8 and JDK11 tests in
> PR
> >> pipeline is twice the chance to encounter possible flaky failures
> (usually
> >> unrelated to the commit itself).
> >>
> >>
> >>
>
>
Re: [DISCUSS] reduce PR checks to JDK11 only
Posted by Owen Nichols <on...@pivotal.io>.
I’ve created a PR for this, please give it a +1: https://github.com/apache/geode/pull/3598
> On May 16, 2019, at 11:12 AM, Anilkumar Gingade <ag...@pivotal.io> wrote:
>
> Make sense to me...Looking at the probability of breaking specific to, jdk8
> and jdk11 through a commit.
>
>
> On Wed, May 15, 2019 at 6:09 PM Owen Nichols <on...@pivotal.io> wrote:
>
>> Currently every PR commit triggers both JDK8 and JDK11 versions of each
>> test job. I propose that we can eliminate the JDK8 version of each check.
>> In the extremely rare case where a code change breaks on Java 8 but works
>> fine on Java 11, it would still be caught by the main pipeline (just as
>> Windows failures are caught only in the main pipeline).
>>
>> The only tangible effect today of running both JDK8 and JDK11 tests in PR
>> pipeline is twice the chance to encounter possible flaky failures (usually
>> unrelated to the commit itself).
>>
>>
>>
Re: [DISCUSS] reduce PR checks to JDK11 only
Posted by Anilkumar Gingade <ag...@pivotal.io>.
Make sense to me...Looking at the probability of breaking specific to, jdk8
and jdk11 through a commit.
On Wed, May 15, 2019 at 6:09 PM Owen Nichols <on...@pivotal.io> wrote:
> Currently every PR commit triggers both JDK8 and JDK11 versions of each
> test job. I propose that we can eliminate the JDK8 version of each check.
> In the extremely rare case where a code change breaks on Java 8 but works
> fine on Java 11, it would still be caught by the main pipeline (just as
> Windows failures are caught only in the main pipeline).
>
> The only tangible effect today of running both JDK8 and JDK11 tests in PR
> pipeline is twice the chance to encounter possible flaky failures (usually
> unrelated to the commit itself).
>
>
>