hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Peter Vary <pv...@cloudera.com>
Subject Re: [DISCUSS] Pre-commit tests before commits
Date Fri, 14 Oct 2016 20:07:08 GMT
About formatting changes we could do it with at least 2 different approach:
Hadoop way - the precommit script which only analyzes the code, and raises a red flag if there
is more error in the patched file than before the patch - no extra effort needed for it other
than creating the infrastructure, and existing files/lines should not be changed which is
in line with the current practice.
Pig way - build a hive without formatting, and then with the formatting, and verify that the
resulting jar-s are the same in binary level. Sounds quiet safe, but could cause a lot of
pain later if someone wants to follow the code changes through the “reformat” barrier.
I think this approach has some more pitfalls, like multiple build environment checks, different
artifacts, etc.
I personally prefer the “Hadoop way”, but if someone has an even better idea I am all
ears/eyes :)

> On Oct 14, 2016, at 9:46 PM, Eugene Koifman <ekoifman@hortonworks.com> wrote:
> 
> Making global formatting changes will create a patch that changes almost
> every line and make git annotate useless which makes figuring out history
> of
> changes difficult.
> 
> Perhaps disabling tests should require a separate +1 (ideally from initial
> author).
> 
> 
> On 10/14/16, 12:27 PM, "Siddharth Seth" <sseth@apache.org <mailto:sseth@apache.org>>
wrote:
> 
>> Once we agree upon what the flow should be, I'm also in favor of reverting
>> patches which break things. Such commits cause problems for all subsequent
>> patches - where multiple people end up debugging the test.
>> In terms of disabling tests - we may want to consider how often the test
>> fails, before disabling them. There are flaky tests - and I suspect we'll
>> end up with a lot of disabled tests if a test is disabled for a single
>> failure.
>> There have been offline suggestions about adding findbugs analysis, rat
>> checks etc. The indentation checks, I think, falls into the same category.
>> Would be really nice to hear from others as well. These changes will be
>> painful to start with - but should help once the tests stabilize further.
>> 
>> On Fri, Oct 14, 2016 at 2:01 AM, Peter Vary <pvary@cloudera.com> wrote:
>> 
>>> +1 from me too.
>>> 
>>> I think it lowers the barrier for the contributors, if there are clean
>>> and
>>> easy to follow rules for adding a patch. I had very good experience with
>>> the Hadoop commit flow where only ³all green² results are accepted.
>>> 
>>> If we are thinking about changes in the commit flow, it would be great
>>> to
>>> run code format checks before and after the patch, and call out any
>>> increase of formatting errors. I see too many nits in the reviews where
>>> the
>>> reviewer points out formatting problems. This could be avoided and the
>>> reviewer can concentrate on the real code instead of formatting.
>>> 
>>> After we decide the new commit flow we should update the documentation
>>> on
>>> the wiki as well.
>>> 
>>> Of course I am happy to help out with any of the related tasks too, so
>>> if
>>> you need my help just say so :)
>>> 
>>> Thanks,
>>> Peter
>>> 
>>>> On Oct 14, 2016, at 10:29 AM, Zsombor Klara
>>> <zsombor.klara@cloudera.com>
>>> wrote:
>>>> 
>>>> +1 from my side as well. I also like the suggestion to revert a patch
>>>> causing new test failures instead of expecting the owner to fix the
>>> issues
>>>> in follow up jiras.
>>>> 
>>>> And I would also have a borderline "radical" suggestion.. if a flaky
>>> test
>>>> is failing and the committer isn't sure at a glance that the failure
>>> is
>>>> benign (stat diff failure for example I would consider low risk, race
>>>> conditions on the other hand high risk), put an ignore annotation on
>>> it.
>>> In
>>>> my view a flaky test is pretty much useless. If it breaks people will
>>> just
>>>> ignore it (because it fails so often...) and anyway how can you be
>>> sure
>>>> that the green run is the standard and the failure is the exception
>>> and
>>> not
>>>> the other way around. We have thousands of tests, ignoring a dozen
>>> won't
>>>> decrease coverage significantly and will take us that much closer to a
>>>> point where we can demand a green pre-commit run.
>>>> 
>>>> Thanks
>>>> 
>>>> On Fri, Oct 14, 2016 at 9:45 AM, Prasanth Jayachandran <
>>>> pjayachandran@hortonworks.com> wrote:
>>>> 
>>>>> +1 on the proposal. Adding more to it.
>>>>> 
>>>>> A lot of time has been spent on improving the test runtime and
>>> bringing
>>>>> down the flaky tests.
>>>>> Following jiras should give an overview of the effort involved
>>>>> https://issues.apache.org/jira/browse/HIVE-14547
>>>>> https://issues.apache.org/jira/browse/HIVE-13503
>>>>> 
>>>>> Committers please ensure that the reported failures are absolutely
>>> not
>>>>> related
>>>>> to the patch before committing it.
>>>>> 
>>>>> I would also propose the following to maintain a clean and some tips
>>> to
>>>>> maintain fast test runs
>>>>> 
>>>>> 1) Revert patch that is causing a failure. It should be the
>>> responsibility
>>>>> of
>>>>> the contributor to make sure the patch is not causing any failures.
>>> I am
>>>>> against creating follow ups
>>>>> for fixing test failures usually because it gets ignored or it gets
>>> lower
>>>>> priority causing wasted effort
>>>>> and time for failure analysis for every other developers waiting to
>>> commit
>>>>> their patch.
>>>>> 
>>>>> 2) +1 from reviewers AFTER a clean green run. Or if a reviewer is
>>>>> convinced that test failures are unrelated.
>>>>> May be we should stop conditional +1s and wait for clean green run.
>>>>> 
>>>>> 3) Avoid slow tests (there is jira to print out runtime of newly
>>> added
>>>>> tests). In general, its good
>>>>> to have many smaller tests as opposed to single big tests. If the
>>> qfile
>>> or
>>>>> junit test is big, splitting it
>>>>> up will help in parallelizing them and avoiding stragglers.
>>>>> 
>>>>> 4) Avoid adding tests to MiniMr (slowest of all).
>>>>> 
>>>>> 5) Try to keep the test runtime (see surefire-report to get correct
>>>>> runtime without initialization time) under a minute.
>>>>> 
>>>>> 6) Avoid adding more read-only tables to init script as this will
>>> increase
>>>>> the initialization time.
>>>>> 
>>>>> 7) If the test case does not require explain plan then avoid it as
>>> most
>>>>> failures are explain diffs.
>>>>> 
>>>>> 8) If the test case requires explain and if it does not depend on
>>> table
>>> or
>>>>> partition stats explicitly set stats for the table or partition.
>>>>> Explicitly setting stats will avoid expensive stats computation time
>>> and
>>>>> avoids flakiness due to stats diff.
>>>>> 
>>>>> 9) Prefer jUnit over qtest.
>>>>> 
>>>>> 10) Add explicitly timeout for jUnit test to avoid indefinite
>>> hanging of
>>>>> tests (surefire timeouts after 40 mins)
>>>>> 
>>>>> Thoughts?
>>>>> 
>>>>> Thanks
>>>>> Prasanth
>>>>> 
>>>>> On Oct 13, 2016, at 11:10 PM, Siddharth Seth
>>> <sseth@apache.org<mailto:
>>>>> sseth@apache.org>> wrote:
>>>>> 
>>>>> There's been a lot of work to make the test runs faster, as well as
>>> more
>>>>> reliable via HIVE-14547, HIVE-13503, and several other jiras. Test
>>> runtimes
>>>>> are around the 1 hour mark, and going down. There were a few green
>>>>> pre-commit runs (after years?). At the same time, there's still some
>>> flaky
>>>>> tests.
>>>>> 
>>>>> We really should try to keep the test runtimes down, as well as the
>>> number
>>>>> of failures - so that the pre-commit runs can provide useful
>>> information.
>>>>> 
>>>>> I'm not sure what the current approach w.r.t precommit runs before a
>>>>> commit. What I've seen in other projects is that the pre-commit
>>> needs to
>>>>> run, and come back clean (mostly) before a commit goes in. Between
>>> what
>>>>> used to be 5 day wait times, and inconsistent runs - I don't think
>>> this
>>> is
>>>>> always followed in Hive.
>>>>> 
>>>>> It'll be useful to start relying on pre-commit test results again.
>>> Given
>>>>> the flaky tests, I'd suggest the following
>>>>> 1. Pre-commit must be run on a patch before committing (with very few
>>>>> exceptions)
>>>>> 2. A green test run is ideal
>>>>> 3. In case there are failures - keep track of these as sub-jiras
>>> under a
>>>>> flaky test umbrella jira (Some under HIVE-14547 already) - to be
>>> eventually
>>>>> fixed.
>>>>> 4. Before committing - cite relevant jiras for a flaky test (create
>>> and
>>>>> cite if it doesn't already exist).
>>>>> 
>>>>> This should help us build up a list of flaky tests over various runs,
>>> which
>>>>> will hopefully get fixed at some point.
>>>>> 
>>>>> Thoughts?
>>>>> 
>>>>> Thanks,
>>>>> Sid


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