IMPORTANT: testing patches for branches

[Allen Wittenauer]

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. :)

Re: IMPORTANT: testing patches for branches

[Vinod Kumar 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  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. :)
> 
> 

Re: IMPORTANT: testing patches for branches

[Allen Wittenauer]

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  
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  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. :)
>> 
>> 
> 

Re: IMPORTANT: testing patches for branches

[Allen Wittenauer]

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  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 
>  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  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. :)
>>> 
>>> 
>> 
> 

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  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  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 <
> vino...@hortonworks.com> 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 
> 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

RE: IMPORTANT: testing patches for branches

[Zheng, Kai]

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:a...@altiscale.com] 
Sent: Thursday, April 23, 2015 3:35 AM
To: common-dev@hadoop.apache.org
Cc: yarn-...@hadoop.apache.org; mapreduce-...@hadoop.apache.org; 
hdfs-...@hadoop.apache.org
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. :)

Re: IMPORTANT: testing patches for branches

[Allen Wittenauer]

On Apr 22, 2015, at 11:34 PM, Zheng, Kai  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.

Re: IMPORTANT: testing patches for branches

[Steve Loughran]

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  wrote:
> 
> 
> 
> 
> 
> On Apr 22, 2015, at 11:34 PM, Zheng, Kai  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.

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 
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  wrote:
> >
> >
> >
> >
> >
> > On Apr 22, 2015, at 11:34 PM, Zheng, Kai  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.
>
>

RE: IMPORTANT: testing patches for branches

[Brahma Reddy Battula]

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 [sidharta.apa...@gmail.com]
Sent: Friday, April 24, 2015 12:27 AM
To: common-dev@hadoop.apache.org
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 
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  wrote:
> >
> >
> >
> >
> >
> > On Apr 22, 2015, at 11:34 PM, Zheng, Kai  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.
>
>

Re: IMPORTANT: testing patches for branches

[Allen Wittenauer]

On Apr 24, 2015, at 3:09 AM, Brahma Reddy Battula 
 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.

Re: IMPORTANT: testing patches for branches

[Allen Wittenauer]

On Apr 23, 2015, at 7:57 PM, Sidharta Seethana  
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.  

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
 unless scope is being
increased.

thanks,
-Sidharta

On Fri, Apr 24, 2015 at 12:55 AM, Allen Wittenauer  wrote:

>
>
>
> On Apr 23, 2015, at 7:57 PM, Sidharta Seethana 
> 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.

RE: IMPORTANT: testing patches for branches

[Zheng, Kai]

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:a...@altiscale.com] 
Sent: Thursday, April 23, 2015 7:22 PM
To: common-dev@hadoop.apache.org
Cc: hdfs-...@hadoop.apache.org; mapreduce-...@hadoop.apache.org; 
yarn-...@hadoop.apache.org
Subject: Re: IMPORTANT: testing patches for branches

On Apr 22, 2015, at 11:34 PM, Zheng, Kai  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.