IMPORTANT: testing patches for branches

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

IMPORTANT: testing patches for branches

Allen Wittenauer-4
       
Hey gang,

        Just so everyone is aware, if you are working on a patch for either a feature branch or a major branch, if you name the patch with the branch name following the spec in HowToContribute (and a few other ways… test-patch tries to figure it out!), test-patch.sh *should* be switching the repo over to that branch for testing.

        For example,  naming a patch foo-branch-2.01.patch should get tested on branch-2.  Naming a patch foo-HDFS-7285.00.patch should get tested on the HDFS-7285 branch.

        This hopefully means that there should really be no more ‘blind’ +1’s to patches that go to branches.  The “we only test against trunk” argument is no longer valid. :)


Reply | Threaded
Open this post in threaded view
|

Re: IMPORTANT: testing patches for branches

Vinod Vavilapalli
Does this mean HADOOP-7435 is no longer needed / closeable as dup?

Thanks
+Vinod

On Apr 22, 2015, at 12:34 PM, Allen Wittenauer <[hidden email]> wrote:

>
> Hey gang,
>
> Just so everyone is aware, if you are working on a patch for either a feature branch or a major branch, if you name the patch with the branch name following the spec in HowToContribute (and a few other ways… test-patch tries to figure it out!), test-patch.sh *should* be switching the repo over to that branch for testing.
>
> For example,  naming a patch foo-branch-2.01.patch should get tested on branch-2.  Naming a patch foo-HDFS-7285.00.patch should get tested on the HDFS-7285 branch.
>
> This hopefully means that there should really be no more ‘blind’ +1’s to patches that go to branches.  The “we only test against trunk” argument is no longer valid. :)
>
>

Reply | Threaded
Open this post in threaded view
|

Re: IMPORTANT: testing patches for branches

Allen Wittenauer-4

More than likely. It probably needs more testing (esp under Jenkins).  

It should be noted that the code in test-patch.sh has lots of problems with branch-0, minor, and micro releases.  But for major releases, it seems to work well for me. :)  

On Apr 22, 2015, at 8:45 PM, Vinod Kumar Vavilapalli <[hidden email]> wrote:

> Does this mean HADOOP-7435 is no longer needed / closeable as dup?
>
> Thanks
> +Vinod
>
> On Apr 22, 2015, at 12:34 PM, Allen Wittenauer <[hidden email]> wrote:
>
>>
>> Hey gang,
>>
>> Just so everyone is aware, if you are working on a patch for either a feature branch or a major branch, if you name the patch with the branch name following the spec in HowToContribute (and a few other ways… test-patch tries to figure it out!), test-patch.sh *should* be switching the repo over to that branch for testing.
>>
>> For example,  naming a patch foo-branch-2.01.patch should get tested on branch-2.  Naming a patch foo-HDFS-7285.00.patch should get tested on the HDFS-7285 branch.
>>
>> This hopefully means that there should really be no more ‘blind’ +1’s to patches that go to branches.  The “we only test against trunk” argument is no longer valid. :)
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: IMPORTANT: testing patches for branches

Allen Wittenauer-4

Oh, this is also in the release notes, but one can use a git reference # as well. :) (with kudos to OOM for the idea.)

On Apr 22, 2015, at 8:57 PM, Allen Wittenauer <[hidden email]> wrote:

>
> More than likely. It probably needs more testing (esp under Jenkins).  
>
> It should be noted that the code in test-patch.sh has lots of problems with branch-0, minor, and micro releases.  But for major releases, it seems to work well for me. :)  
>
> On Apr 22, 2015, at 8:45 PM, Vinod Kumar Vavilapalli <[hidden email]> wrote:
>
>> Does this mean HADOOP-7435 is no longer needed / closeable as dup?
>>
>> Thanks
>> +Vinod
>>
>> On Apr 22, 2015, at 12:34 PM, Allen Wittenauer <[hidden email]> wrote:
>>
>>>
>>> Hey gang,
>>>
>>> Just so everyone is aware, if you are working on a patch for either a feature branch or a major branch, if you name the patch with the branch name following the spec in HowToContribute (and a few other ways… test-patch tries to figure it out!), test-patch.sh *should* be switching the repo over to that branch for testing.
>>>
>>> For example,  naming a patch foo-branch-2.01.patch should get tested on branch-2.  Naming a patch foo-HDFS-7285.00.patch should get tested on the HDFS-7285 branch.
>>>
>>> This hopefully means that there should really be no more ‘blind’ +1’s to patches that go to branches.  The “we only test against trunk” argument is no longer valid. :)
>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: IMPORTANT: testing patches for branches

Niels Basjes
Perhaps a script is due that creates the patch file with -exactly- the
right name.
Something like what HBase has as dev-support/make_patch.sh perhaps?

On Wed, Apr 22, 2015 at 10:30 PM, Allen Wittenauer <[hidden email]> wrote:

>
> Oh, this is also in the release notes, but one can use a git reference #
> as well. :) (with kudos to OOM for the idea.)
>
> On Apr 22, 2015, at 8:57 PM, Allen Wittenauer <[hidden email]> wrote:
>
> >
> > More than likely. It probably needs more testing (esp under Jenkins).
> >
> > It should be noted that the code in test-patch.sh has lots of problems
> with branch-0, minor, and micro releases.  But for major releases, it seems
> to work well for me. :)
> >
> > On Apr 22, 2015, at 8:45 PM, Vinod Kumar Vavilapalli <
> [hidden email]> wrote:
> >
> >> Does this mean HADOOP-7435 is no longer needed / closeable as dup?
> >>
> >> Thanks
> >> +Vinod
> >>
> >> On Apr 22, 2015, at 12:34 PM, Allen Wittenauer <[hidden email]>
> wrote:
> >>
> >>>
> >>> Hey gang,
> >>>
> >>>     Just so everyone is aware, if you are working on a patch for
> either a feature branch or a major branch, if you name the patch with the
> branch name following the spec in HowToContribute (and a few other ways…
> test-patch tries to figure it out!), test-patch.sh *should* be switching
> the repo over to that branch for testing.
> >>>
> >>>     For example,  naming a patch foo-branch-2.01.patch should get
> tested on branch-2.  Naming a patch foo-HDFS-7285.00.patch should get
> tested on the HDFS-7285 branch.
> >>>
> >>>     This hopefully means that there should really be no more ‘blind’
> +1’s to patches that go to branches.  The “we only test against trunk”
> argument is no longer valid. :)
> >>>
> >>>
> >>
> >
>
>


--
Best regards / Met vriendelijke groeten,

Niels Basjes
Reply | Threaded
Open this post in threaded view
|

RE: IMPORTANT: testing patches for branches

Zheng, Kai
In reply to this post by Allen Wittenauer-4
Hi Allen,

This sounds great.

>> Naming a patch foo-HDFS-7285.00.patch should get tested on the HDFS-7285 branch.
Does it happen locally in developer's machine when running test-patch.sh, or also mean something in Hadoop Jenkins building when a JIRA becoming patch available? Thanks.

Regards,
Kai

-----Original Message-----
From: Allen Wittenauer [mailto:[hidden email]]
Sent: Thursday, April 23, 2015 3:35 AM
To: [hidden email]
Cc: [hidden email]; [hidden email]; [hidden email]
Subject: IMPORTANT: testing patches for branches

       
Hey gang,

        Just so everyone is aware, if you are working on a patch for either a feature branch or a major branch, if you name the patch with the branch name following the spec in HowToContribute (and a few other ways... test-patch tries to figure it out!), test-patch.sh *should* be switching the repo over to that branch for testing.

        For example,  naming a patch foo-branch-2.01.patch should get tested on branch-2.  Naming a patch foo-HDFS-7285.00.patch should get tested on the HDFS-7285 branch.

        This hopefully means that there should really be no more 'blind' +1's to patches that go to branches.  The "we only test against trunk" argument is no longer valid. :)


Reply | Threaded
Open this post in threaded view
|

Re: IMPORTANT: testing patches for branches

Allen Wittenauer-4




On Apr 22, 2015, at 11:34 PM, Zheng, Kai <[hidden email]> wrote:

> Hi Allen,
>
> This sounds great.
>
>>> Naming a patch foo-HDFS-7285.00.patch should get tested on the HDFS-7285 branch.
> Does it happen locally in developer's machine when running test-patch.sh, or also mean something in Hadoop Jenkins building when a JIRA becoming patch available? Thanks.


        Both, now that a fix has been committed last night (there was a bug in the Jenkins handling).

        Given a patch name or URL, Jenkins and even running locally will try a few different methods to figure out which branch to use  out.  Note that a branch name of ‘gitX’ where X is a valid git reference also works to force a patch to start at a particular commit.

        For local use, you’ll want to use a ‘spare’ copy of the source tree via the —basedir option and use the —resetrepo flag.  That will enable Jenkins-like behavior and gives it permission to make modifications and effectively nuke any changes in the source tree you point it at.  (Basically the opposite of the —dirty-workspace flag).  If you want to force a branch (for whatever reason, including where the branch can’t be figured out), you can use the —branch option.

        If you don’t use —resetrepo, test-patch.sh will warn that it thinks the wrong branch is being used but will push on anyway.

        In any case, the result of what it thinks the branch is/should be will be in the summary output at the bottom along with the git ref that it specifically used for the test.
Reply | Threaded
Open this post in threaded view
|

Re: IMPORTANT: testing patches for branches

Steve Loughran-3

1. I really like the new patch process, especially the at-a-glance summary
2. I think being -1 on whitespace is overkill; Its just part of my " git apply -p 0 -3 --verbose --whitespace=fix  " action. Accordingly, I won't reject patches on whitespace alone.
3. if checkstyle is complaining, how to track it down? As example, I don't see much from:
https://builds.apache.org/job/PreCommit-HADOOP-Build/6167/artifact/patchprocess/checkstyle-result-diff.txt


> On 23 Apr 2015, at 12:21, Allen Wittenauer <[hidden email]> wrote:
>
>
>
>
>
> On Apr 22, 2015, at 11:34 PM, Zheng, Kai <[hidden email]> wrote:
>
>> Hi Allen,
>>
>> This sounds great.
>>
>>>> Naming a patch foo-HDFS-7285.00.patch should get tested on the HDFS-7285 branch.
>> Does it happen locally in developer's machine when running test-patch.sh, or also mean something in Hadoop Jenkins building when a JIRA becoming patch available? Thanks.
>
>
> Both, now that a fix has been committed last night (there was a bug in the Jenkins handling).
>
> Given a patch name or URL, Jenkins and even running locally will try a few different methods to figure out which branch to use  out.  Note that a branch name of ‘gitX’ where X is a valid git reference also works to force a patch to start at a particular commit.
>
> For local use, you’ll want to use a ‘spare’ copy of the source tree via the —basedir option and use the —resetrepo flag.  That will enable Jenkins-like behavior and gives it permission to make modifications and effectively nuke any changes in the source tree you point it at.  (Basically the opposite of the —dirty-workspace flag).  If you want to force a branch (for whatever reason, including where the branch can’t be figured out), you can use the —branch option.
>
> If you don’t use —resetrepo, test-patch.sh will warn that it thinks the wrong branch is being used but will push on anyway.
>
> In any case, the result of what it thinks the branch is/should be will be in the summary output at the bottom along with the git ref that it specifically used for the test.

Reply | Threaded
Open this post in threaded view
|

Re: IMPORTANT: testing patches for branches

Sidharta Seethana
About (3.) , a lot of the check style rules seem to be arcane/unnecessary.
Please see : https://issues.apache.org/jira/browse/HADOOP-11869

On Thu, Apr 23, 2015 at 10:45 AM, Steve Loughran <[hidden email]>
wrote:

>
> 1. I really like the new patch process, especially the at-a-glance summary
> 2. I think being -1 on whitespace is overkill; Its just part of my " git
> apply -p 0 -3 --verbose --whitespace=fix  " action. Accordingly, I won't
> reject patches on whitespace alone.
> 3. if checkstyle is complaining, how to track it down? As example, I don't
> see much from:
>
> https://builds.apache.org/job/PreCommit-HADOOP-Build/6167/artifact/patchprocess/checkstyle-result-diff.txt
>
>
> > On 23 Apr 2015, at 12:21, Allen Wittenauer <[hidden email]> wrote:
> >
> >
> >
> >
> >
> > On Apr 22, 2015, at 11:34 PM, Zheng, Kai <[hidden email]> wrote:
> >
> >> Hi Allen,
> >>
> >> This sounds great.
> >>
> >>>> Naming a patch foo-HDFS-7285.00.patch should get tested on the
> HDFS-7285 branch.
> >> Does it happen locally in developer's machine when running
> test-patch.sh, or also mean something in Hadoop Jenkins building when a
> JIRA becoming patch available? Thanks.
> >
> >
> >       Both, now that a fix has been committed last night (there was a
> bug in the Jenkins handling).
> >
> >       Given a patch name or URL, Jenkins and even running locally will
> try a few different methods to figure out which branch to use  out.  Note
> that a branch name of ‘gitX’ where X is a valid git reference also works to
> force a patch to start at a particular commit.
> >
> >       For local use, you’ll want to use a ‘spare’ copy of the source
> tree via the —basedir option and use the —resetrepo flag.  That will enable
> Jenkins-like behavior and gives it permission to make modifications and
> effectively nuke any changes in the source tree you point it at.
> (Basically the opposite of the —dirty-workspace flag).  If you want to
> force a branch (for whatever reason, including where the branch can’t be
> figured out), you can use the —branch option.
> >
> >       If you don’t use —resetrepo, test-patch.sh will warn that it
> thinks the wrong branch is being used but will push on anyway.
> >
> >       In any case, the result of what it thinks the branch is/should be
> will be in the summary output at the bottom along with the git ref that it
> specifically used for the test.
>
>
Reply | Threaded
Open this post in threaded view
|

RE: IMPORTANT: testing patches for branches

Brahma Reddy

Overall approach and summary is very good... well done

Apart from steve pointed which I had also noticed, following one also i had seen where I did not seen any clue..

The patch artifact directory on has been removed!
This is a fatal error for test-patch.sh. Aborting.
Jenkins (node H8) information at https://builds.apache.org/job/PreCommit-YARN-Build/7477/ may provide some hints.  ( did I miss here something )

Atlast one suggestion is : can we remove whitespace check..?

Thanks & Regards
Brahma Reddy Battula

________________________________________
From: Sidharta Seethana [[hidden email]]
Sent: Friday, April 24, 2015 12:27 AM
To: [hidden email]
Subject: Re: IMPORTANT: testing patches for branches

About (3.) , a lot of the check style rules seem to be arcane/unnecessary.
Please see : https://issues.apache.org/jira/browse/HADOOP-11869

On Thu, Apr 23, 2015 at 10:45 AM, Steve Loughran <[hidden email]>
wrote:

>
> 1. I really like the new patch process, especially the at-a-glance summary
> 2. I think being -1 on whitespace is overkill; Its just part of my " git
> apply -p 0 -3 --verbose --whitespace=fix  " action. Accordingly, I won't
> reject patches on whitespace alone.
> 3. if checkstyle is complaining, how to track it down? As example, I don't
> see much from:
>
> https://builds.apache.org/job/PreCommit-HADOOP-Build/6167/artifact/patchprocess/checkstyle-result-diff.txt
>
>
> > On 23 Apr 2015, at 12:21, Allen Wittenauer <[hidden email]> wrote:
> >
> >
> >
> >
> >
> > On Apr 22, 2015, at 11:34 PM, Zheng, Kai <[hidden email]> wrote:
> >
> >> Hi Allen,
> >>
> >> This sounds great.
> >>
> >>>> Naming a patch foo-HDFS-7285.00.patch should get tested on the
> HDFS-7285 branch.
> >> Does it happen locally in developer's machine when running
> test-patch.sh, or also mean something in Hadoop Jenkins building when a
> JIRA becoming patch available? Thanks.
> >
> >
> >       Both, now that a fix has been committed last night (there was a
> bug in the Jenkins handling).
> >
> >       Given a patch name or URL, Jenkins and even running locally will
> try a few different methods to figure out which branch to use  out.  Note
> that a branch name of ‘gitX’ where X is a valid git reference also works to
> force a patch to start at a particular commit.
> >
> >       For local use, you’ll want to use a ‘spare’ copy of the source
> tree via the —basedir option and use the —resetrepo flag.  That will enable
> Jenkins-like behavior and gives it permission to make modifications and
> effectively nuke any changes in the source tree you point it at.
> (Basically the opposite of the —dirty-workspace flag).  If you want to
> force a branch (for whatever reason, including where the branch can’t be
> figured out), you can use the —branch option.
> >
> >       If you don’t use —resetrepo, test-patch.sh will warn that it
> thinks the wrong branch is being used but will push on anyway.
> >
> >       In any case, the result of what it thinks the branch is/should be
> will be in the summary output at the bottom along with the git ref that it
> specifically used for the test.
>
>
Reply | Threaded
Open this post in threaded view
|

Re: IMPORTANT: testing patches for branches

Allen Wittenauer-4

On Apr 24, 2015, at 3:09 AM, Brahma Reddy Battula <[hidden email]> wrote:

> The patch artifact directory on has been removed!
> This is a fatal error for test-patch.sh. Aborting.
> Jenkins (node H8) information at https://builds.apache.org/job/PreCommit-YARN-Build/7477/ may provide some hints.  ( did I miss here something )
>


        This is the new message that was added in the addendum patch.  (It clearly needs some grammar work. lol) test-patch.sh uses this when it detects that something else has removed the artifact directory on the jenkins server. Prior to this change, test-patch (both old and new) would silently fail, leaving everyone wondering where the patch result went.  Now you at least get some notice that Jenkins (or as some suspect, a broken test) screwed up.  It also acts as an early abort, rather than attempting to continue to run tests that will also fail.
Reply | Threaded
Open this post in threaded view
|

Re: IMPORTANT: testing patches for branches

Allen Wittenauer-4
In reply to this post by Sidharta Seethana



On Apr 23, 2015, at 7:57 PM, Sidharta Seethana <[hidden email]> wrote:

> About (3.) , a lot of the check style rules seem to be arcane/unnecessary.
> Please see : https://issues.apache.org/jira/browse/HADOOP-11869


a) I've closed it as a dupe of HADOOP-11866 to keep everything in one place.

b) I've had HADOOP-11778 open for a while to update checkstyle to a more modern version…. which will also likely fix HADOOP-11546.

c) According to our commit guidelines, checkstyle is a requirement for commitment.  If we want to remove that requirement, we need to modify the guidelines and comment out the registration line in the checkstyle.sh plugin. We've been ignoring it for whatever reasons, likely because the code in the old test-patch.sh was pretty broken to the point of being disabled.


        Personally, while some view this as a "minor formatting issue", it reflects poorly on the project to have every file formatted differently. check style is meant to enforce those rules.   This *is* a quality check.

        Given that branch-2 went from "stable" (~2.4) to "beta" (~2.6) to  "alpha" (officially 2.7), well… I guess I shouldn't be surprised that quality is going down though.  
Reply | Threaded
Open this post in threaded view
|

Re: IMPORTANT: testing patches for branches

Sidharta Seethana
Allen,

I am not questioning whether checkstyle is a requirement. My concern is
that some of the rules in there are arcane/unnecessary - we should use a
better list of rules, in my opinion. This is why I filed HADOOP-11869. As
pointed out by Jason, this is not a dupe of HADOOP-11866
<https://issues.apache.org/jira/browse/HADOOP-11866> unless scope is being
increased.

thanks,
-Sidharta

On Fri, Apr 24, 2015 at 12:55 AM, Allen Wittenauer <[hidden email]> wrote:

>
>
>
> On Apr 23, 2015, at 7:57 PM, Sidharta Seethana <[hidden email]>
> wrote:
>
> > About (3.) , a lot of the check style rules seem to be
> arcane/unnecessary.
> > Please see : https://issues.apache.org/jira/browse/HADOOP-11869
>
>
> a) I've closed it as a dupe of HADOOP-11866 to keep everything in one
> place.
>
> b) I've had HADOOP-11778 open for a while to update checkstyle to a more
> modern version…. which will also likely fix HADOOP-11546.
>
> c) According to our commit guidelines, checkstyle is a requirement for
> commitment.  If we want to remove that requirement, we need to modify the
> guidelines and comment out the registration line in the checkstyle.sh
> plugin. We've been ignoring it for whatever reasons, likely because the
> code in the old test-patch.sh was pretty broken to the point of being
> disabled.
>
>
>         Personally, while some view this as a "minor formatting issue", it
> reflects poorly on the project to have every file formatted differently.
> check style is meant to enforce those rules.   This *is* a quality check.
>
>         Given that branch-2 went from "stable" (~2.4) to "beta" (~2.6) to
> "alpha" (officially 2.7), well… I guess I shouldn't be surprised that
> quality is going down though.
Reply | Threaded
Open this post in threaded view
|

RE: IMPORTANT: testing patches for branches

Zheng, Kai
In reply to this post by Allen Wittenauer-4
Thanks Allen for the great work. I tried in HADOOP-11847 (branch HDFS-7285) and it went well, very helpfully!

Regards,
Kai

-----Original Message-----
From: Allen Wittenauer [mailto:[hidden email]]
Sent: Thursday, April 23, 2015 7:22 PM
To: [hidden email]
Cc: [hidden email]; [hidden email]; [hidden email]
Subject: Re: IMPORTANT: testing patches for branches

On Apr 22, 2015, at 11:34 PM, Zheng, Kai <[hidden email]> wrote:

> Hi Allen,
>
> This sounds great.
>
>>> Naming a patch foo-HDFS-7285.00.patch should get tested on the HDFS-7285 branch.
> Does it happen locally in developer's machine when running test-patch.sh, or also mean something in Hadoop Jenkins building when a JIRA becoming patch available? Thanks.


        Both, now that a fix has been committed last night (there was a bug in the Jenkins handling).

        Given a patch name or URL, Jenkins and even running locally will try a few different methods to figure out which branch to use  out.  Note that a branch name of 'gitX' where X is a valid git reference also works to force a patch to start at a particular commit.

        For local use, you'll want to use a 'spare' copy of the source tree via the -basedir option and use the -resetrepo flag.  That will enable Jenkins-like behavior and gives it permission to make modifications and effectively nuke any changes in the source tree you point it at.  (Basically the opposite of the -dirty-workspace flag).  If you want to force a branch (for whatever reason, including where the branch can't be figured out), you can use the -branch option.

        If you don't use -resetrepo, test-patch.sh will warn that it thinks the wrong branch is being used but will push on anyway.

        In any case, the result of what it thinks the branch is/should be will be in the summary output at the bottom along with the git ref that it specifically used for the test.