[DISCUSS] Add extra check for Avatica to run tests with different timezone

[Alessandro Solimando <al...@gmail.com>]

Hello everyone,
we have recently had an issue for a change in Avatica which was not working
for all timezones that almost slipped through (see CALCITE-4600
<https://issues.apache.org/jira/browse/CALCITE-4600>) if it was not for a
local run which happened to be in a different timezone.

As for Calcite, also Avatica has code paths which are susceptible to depend
on the local user timezone (notably the time-related data types), in the
current state CI won't necessarily catch misbehaviour related to that.

I think it would be worth adding at least an additional CI check using a
US/Asia timezone. Avatica tests are quick to run, so I don't think it would
be a big issue.

What do you think?

Best regards,
Alessandro

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Josh Elser <el...@apache.org>]

Having had to debug test failures for projects which previously did 
(simple) randomized testing, I am also not a fan of randomization. If 
there is a way to deterministically re-run a specific configuration, 
this goes a long way.

I am more worried about debt in maintaining the complexity of a 
high-cardinality test matrix. If we are randomly choosing one "diagonal" 
(to steal Julian's term), we may miss a breakage from some code change 
in the same way we might catch one.

I'm of the mindset that we cannot possibly test all variants. My opinion 
is that we should be mindful of the incompatibilities which we have 
observed with Avatica over the years and use that to define what is a 
"meaningful" thing to test.

That said (going through Vlad's list), I don't think testing various 
OS's is important. We should pick the JDK (or JDKs) which our devs/users 
rely on today -- for me, that's Oracle or OpenJDK. I would also not want 
to test against more than a few versions of Guava. Same goes for 
timezones (picking two or three). For all of these, as long as we don't 
get into hours-long precommit duration and we have the CPU in order to 
run the tests, I'm happy with whatever.

On 10/2/21 6:03 PM, Stamatis Zampetakis wrote:
> Hi all,
> 
> I like the idea of extending our test coverage and the fact people are
> putting effort on this topic.
> 
> I don't really mind having randomized or fixed CI checks as long as:
> 1. it doesn't break too often due to the environment;
> 2. it is capable to pinpoint regressions/problems on specific commits;
> 3. it doesn't take overly long to run;
> 4. failures are reproducible.
> 
> I am a bit skeptical with respect to randomization mostly about point 1 but
> we can test it and see how it goes.
> Are other projects using this? How does it look so far?
> 
>  From my perspective, I wouldn't opt for everything to be randomized but
> rather amend the fixed CI checks.
> Moreover, ideally I would like errors in randomized checks to not turn up
> as red builds but rather orange, with the semantics being let's log the
> issue but not block commits to master till the issue is resolved. Not sure
> if that's possible, just ideas.
> 
> Also maybe we would like to keep randomization only on master (running it
> once per day) and not on every PR mostly to save some resources.
> 
> Regarding tests on different timezones I find it a very good idea.
> Maybe, it would be better to do that on a per test basis rather than on the
> whole build but given that our unit tests run fast (~3-5min) I guess we can
> afford running everything on different zones.
> If we wanted to make timezone tests more focused we could possibly utilize
> JUnit5 extensions for running certain tests on multiple timezones (ideas
> for the future).
> 
> Best,
> Stamatis
> 
> On Sat, Oct 2, 2021 at 8:53 PM Vladimir Sitnikov <
> sitnikov.vladimir@gmail.com> wrote:
> 
>>> I don’t think we should do randomized CI
>>
>> Then please incorporate:
>> a) Testing with "-XX:+UnlockExperimentalVMOptions -XX:hashCode=2" to ensure
>> the code does not rely on Object#hashCode uniqueness
>> b) different OSes
>> c) Different JDK vendors, versions and JIT implementations (e.g. OpenJ9,
>> Microsoft, Coretto)
>> d) interference of a, b, c, and Guava versions
>>
>> Vladimir
>>
> 

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Stamatis Zampetakis <za...@gmail.com>]

Hi all,

I like the idea of extending our test coverage and the fact people are
putting effort on this topic.

I don't really mind having randomized or fixed CI checks as long as:
1. it doesn't break too often due to the environment;
2. it is capable to pinpoint regressions/problems on specific commits;
3. it doesn't take overly long to run;
4. failures are reproducible.

I am a bit skeptical with respect to randomization mostly about point 1 but
we can test it and see how it goes.
Are other projects using this? How does it look so far?

From my perspective, I wouldn't opt for everything to be randomized but
rather amend the fixed CI checks.
Moreover, ideally I would like errors in randomized checks to not turn up
as red builds but rather orange, with the semantics being let's log the
issue but not block commits to master till the issue is resolved. Not sure
if that's possible, just ideas.

Also maybe we would like to keep randomization only on master (running it
once per day) and not on every PR mostly to save some resources.

Regarding tests on different timezones I find it a very good idea.
Maybe, it would be better to do that on a per test basis rather than on the
whole build but given that our unit tests run fast (~3-5min) I guess we can
afford running everything on different zones.
If we wanted to make timezone tests more focused we could possibly utilize
JUnit5 extensions for running certain tests on multiple timezones (ideas
for the future).

Best,
Stamatis

On Sat, Oct 2, 2021 at 8:53 PM Vladimir Sitnikov <
sitnikov.vladimir@gmail.com> wrote:

> >I don’t think we should do randomized CI
>
> Then please incorporate:
> a) Testing with "-XX:+UnlockExperimentalVMOptions -XX:hashCode=2" to ensure
> the code does not rely on Object#hashCode uniqueness
> b) different OSes
> c) Different JDK vendors, versions and JIT implementations (e.g. OpenJ9,
> Microsoft, Coretto)
> d) interference of a, b, c, and Guava versions
>
> Vladimir
>

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Vladimir Sitnikov <si...@gmail.com>]

>I don’t think we should do randomized CI

Then please incorporate:
a) Testing with "-XX:+UnlockExperimentalVMOptions -XX:hashCode=2" to ensure
the code does not rely on Object#hashCode uniqueness
b) different OSes
c) Different JDK vendors, versions and JIT implementations (e.g. OpenJ9,
Microsoft, Coretto)
d) interference of a, b, c, and Guava versions

Vladimir

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Julian Hyde <jh...@gmail.com>]

I don’t think we should do randomized CI.

> On Oct 2, 2021, at 1:32 PM, Vladimir Sitnikov <si...@gmail.com> wrote:
> 
> Here's a randomized matrix for Avatica:
> https://github.com/apache/calcite-avatica/pull/156
> https://github.com/apache/calcite-avatica/actions/runs/1298777387
> 
> Vladimir

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Vladimir Sitnikov <si...@gmail.com>]

Here's a randomized matrix for Avatica:
https://github.com/apache/calcite-avatica/pull/156
https://github.com/apache/calcite-avatica/actions/runs/1298777387

Vladimir

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Vladimir Sitnikov <si...@gmail.com>]

I suggest to settle on the number of CI jobs we can afford to keep the CI
duration sane, and we generate a random sample. The number of CI jobs might
be different for PR builds and for branch builds.

We have many different interesting timezones, many Guava versions, only 3
OS, many JDKs for each OS, so builing diagonal is not really trivial.

Vladimir

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Julian Hyde <jh...@gmail.com>]

> Do you suggest test all OS, all Guava versions, all timezones in CI?
> That is hard to maintain and it takes CI time

No I don’t suggest that. That seemed to be what you were suggesting!

My suggestion is to test a ‘diagonal’. Suppose we need to test 5 JDK versions, 5 Guava versions, and 5 operating systems. The full matrix is 5 ^ 3 = 125 entries. But if we just test the diagonal, {(jdk #0, guava #0, os #0), … (jdk #4, guava #4, os #4)} then each parameter gets tested once. That’s not perfect, but it’s a huge improvement with a minimal amount of CI resources. We missed some bugs because we were testing only one Guava version and only one time zone.

Infrequently - say once a month or once per release - someone can run the whole matrix.

Julian


> On Oct 1, 2021, at 10:02 AM, Vladimir Sitnikov <si...@gmail.com> wrote:
> 
>> CI should be deterministic
> 
> That is an interesting view.
> One of the approaches that I have not considered earlier is "seed matrix
> randomization from PR index and the branch name".
> That would make the set of jobs consistent for each execution of the same
> PR.
> 
> However, based on my testng/testng experience, it is just fine if the set
> of CI jobs varies.
> The CI job description does clarify what was special (e.g. timezone, JVM,
> OS, etc).
> 
>> And also tests with different Guava versions
> 
> Do you suggest test all OS, all Guava versions, all timezones in CI?
> That is hard to maintain and it takes CI time
> 
>> CI should be deterministic. So that if it breaks, the person to fix it is
> clear
> 
> CI resources are limited, and we can't infinitely scale the build matrix.
> I do not really see why Guava versions are more important than operating
> systems.
> The users should prefer recent Guava versions, and the idea of supporting
> (and testing!) ancient Guava versions is doubtful.
> 
> On the other hand, we might want to verify different cases like
> -XX:hashCode=2 that makes every Object#hashCode() to return 1 always.
> 
> Vladimir

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Vladimir Sitnikov <si...@gmail.com>]

>CI should be deterministic

That is an interesting view.
One of the approaches that I have not considered earlier is "seed matrix
randomization from PR index and the branch name".
That would make the set of jobs consistent for each execution of the same
PR.

However, based on my testng/testng experience, it is just fine if the set
of CI jobs varies.
The CI job description does clarify what was special (e.g. timezone, JVM,
OS, etc).

>And also tests with different Guava versions

Do you suggest test all OS, all Guava versions, all timezones in CI?
That is hard to maintain and it takes CI time

>CI should be deterministic. So that if it breaks, the person to fix it is
clear

CI resources are limited, and we can't infinitely scale the build matrix.
I do not really see why Guava versions are more important than operating
systems.
The users should prefer recent Guava versions, and the idea of supporting
(and testing!) ancient Guava versions is doubtful.

On the other hand, we might want to verify different cases like
-XX:hashCode=2 that makes every Object#hashCode() to return 1 always.

Vladimir

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Julian Hyde <jh...@gmail.com>]

+1 for tests in different time zones. And also tests with different Guava versions. In fact I’m working on it: see https://github.com/julianhyde/calcite-avatica/commits/stage <https://github.com/julianhyde/calcite-avatica/commits/stage> and https://app.travis-ci.com/github/julianhyde/calcite-avatica/builds/238880701 <https://app.travis-ci.com/github/julianhyde/calcite-avatica/builds/238880701>.

-1 for randomized testing as part of CI. CI should be deterministic. So that if it breaks, the person to fix it is clear: it’s generally the last person who changed something.

I support randomized testing via scripts. People can run the script for a couple of hours and log any bugs it uncovers. We already have several such scripts in Calcite (e.g. rex fuzz testing).

Julian


> On Oct 1, 2021, at 3:28 AM, Alessandro Solimando <al...@gmail.com> wrote:
> 
> Thanks Vladimir for the proposal.
> 
> +1 frome me, there are more and more "axes" for testing (OS, locale, JVM,
> timezone, etc.), a randomized approach sounds like a good solution to tame
> the combinatorial explosion.
> 
> Best regards,
> Alessandro
> 
> On Fri, 1 Oct 2021 at 08:32, Vladimir Sitnikov <si...@gmail.com>
> wrote:
> 
>>> Of course it would be fine to just change the timezone for one of more
>>> existing tests which are targeting different JVM versions
>> 
>> Alessandro, team, what do you think of
>> https://github.com/vlsi/github-actions-random-matrix ?
>> 
>> I'm kind of hesitant to introduce more and more /vlsi/ dependencies even
>> though I believe they are the best tools available :)
>> random-matix is a 200 line script that generates build matrix by selecting
>> random values for the axes (e.g. jvm version, os, guava version, etc).
>> 
>> Vladimir
>> 

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Alessandro Solimando <al...@gmail.com>]

Thanks Vladimir for the proposal.

+1 frome me, there are more and more "axes" for testing (OS, locale, JVM,
timezone, etc.), a randomized approach sounds like a good solution to tame
the combinatorial explosion.

Best regards,
Alessandro

On Fri, 1 Oct 2021 at 08:32, Vladimir Sitnikov <si...@gmail.com>
wrote:

> >Of course it would be fine to just change the timezone for one of more
> >existing tests which are targeting different JVM versions
>
> Alessandro, team, what do you think of
> https://github.com/vlsi/github-actions-random-matrix ?
>
> I'm kind of hesitant to introduce more and more /vlsi/ dependencies even
> though I believe they are the best tools available :)
> random-matix is a 200 line script that generates build matrix by selecting
> random values for the axes (e.g. jvm version, os, guava version, etc).
>
> Vladimir
>

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Vladimir Sitnikov <si...@gmail.com>]

>Of course it would be fine to just change the timezone for one of more
>existing tests which are targeting different JVM versions

Alessandro, team, what do you think of
https://github.com/vlsi/github-actions-random-matrix ?

I'm kind of hesitant to introduce more and more /vlsi/ dependencies even
though I believe they are the best tools available :)
random-matix is a 200 line script that generates build matrix by selecting
random values for the axes (e.g. jvm version, os, guava version, etc).

Vladimir

Re: [DISCUSS] Add extra check for Avatica to run tests with different timezone

[Alessandro Solimando <al...@gmail.com>]

Of course it would be fine to just change the timezone for one of more
existing tests which are targeting different JVM versions, no need for a
brand new check.

BR,
Alessandro

On Fri, 1 Oct 2021 at 08:07, Alessandro Solimando <
alessandro.solimando@gmail.com> wrote:

> Hello everyone,
> we have recently had an issue for a change in Avatica which was not
> working for all timezones that almost slipped through (see CALCITE-4600
> <https://issues.apache.org/jira/browse/CALCITE-4600>) if it was not for a
> local run which happened to be in a different timezone.
>
> As for Calcite, also Avatica has code paths which are susceptible to
> depend on the local user timezone (notably the time-related data types), in
> the current state CI won't necessarily catch misbehaviour related to that.
>
> I think it would be worth adding at least an additional CI check using a
> US/Asia timezone. Avatica tests are quick to run, so I don't think it would
> be a big issue.
>
> What do you think?
>
> Best regards,
> Alessandro
>