geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Owen Nichols <onich...@pivotal.io>
Subject Re: [DISCUSS] reduce PR checks to JDK11 only
Date Mon, 20 May 2019 21:04:30 GMT
@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 <jbarrett@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 <rhoughton@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 <onichols@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 <dschneider@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 <onichols@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 <agingade@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 <onichols@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).
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>> 
>>>>> 
>>> 
>>> 


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message