You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by Ted Dunning <te...@gmail.com> on 2011/11/01 00:08:01 UTC
Re: cleanup and subjective patches
How about a middle ground being that aesthetic changes will only be
considered if they bring significant new testing that helps mitigate the
stability risk?
On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <ph...@apache.org> wrote:
> EOD this is not a tool problem, it's a resource issue. We have limited
> committer time available for reviews. IMO Thomas's changes have been
> pretty mechanical so far, they are easy to review/commit. They do
> result in some patch churn but I've been surprised it's been so little
> so far and the code is improving as a result. However we've just
> scratched the surface (the easy stuff), I do have some concerns as we
> make more significant structural changes. Personally I think we need
> to focus on beefing up testing prior to making more significant
> changes.
>
> Ben - We won't get new committers if we limit contributions. We'll
> just dig a deeper hole for ourselves. Granted if we only review/accept
> patches of a particular type (or from a particular person) we'll be in
> a similar situation.
>
> Thomas - Ben has highlighted some good points, if we don't discuss
> these rationally, out in the open, we won't be able to make progress.
> There are other patches from other contributors that we need to
> balance the available resources against.
>
> http://communityovercode.com/over/
>
>
> Personally: my biggest concern is that we keep releasing solid high
> quality code that works in production. I'm seeing patches go in that
> break existing functionality and no one notices. To me the thing we
> should be focusing on is adding testing. Refactoring and such to
> improve/increase testing imo is a net positive.
>
> Patrick
>
> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <te...@gmail.com>
> wrote:
> > Jumping over to Ben's side of the discussion,
> >
> > Git helps with this, but does not eliminate the problem. At some point
> the
> > changes become difficult to understand relative to the new code and may
> > even collide in ways that are difficult to merge.
> >
> > It is true, however, that patches can be kept live using tools like git.
> > That is how I kept the multi stuff alive, but there was at least one
> > tricky merge to be done.
> >
> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <th...@koch.ro> wrote:
> >
> >> Thanks to Ted for replying. I will save the mail I started in the drafts
> >> folder until I've calmed down again.
> >>
> >> Benjamin Reed:
> >> > deprioritizing them doesn't help because the patches themselves bit
> >> > rot. shortly they will not apply and then they will be worthless. the
> >> > poor contributor would then be left with the task of maintaining a
> >> > patch that would never commit.
> >> You should really give GIT a try. I've kept a pipeline of half a douzend
> >> patches filled and current over the last two months while drinking my
> >> morning
> >> coffee.
> >> Likewise have I updated my development branch of over a douzend commits
> >> every
> >> morning against the new ZK trunk.
> >>
> >> Regards,
> >>
> >> Thomas Koch, http://www.koch.ro
> >>
> >
>
RE: cleanup and subjective patches
Posted by "Fournier, Camille F." <Ca...@gs.com>.
Great way of phrasing it, Patrick. I agree. This community member thinks that the period around releases would be best spent concentrating on getting the release out the door, and the period immediately following releases is the best time to concentrate on new features, refactoring, and general code base cleanup.
And Thomas, if there's ever a bug you find but you don't feel enabled in whatever way to fix, feel free to ping me directly and I'm happy to help you get started.
C
-----Original Message-----
From: Patrick Hunt [mailto:phunt@apache.org]
Sent: Tuesday, November 01, 2011 1:32 PM
To: dev@zookeeper.apache.org
Cc: thomas@koch.ro; Benjamin Reed
Subject: Re: cleanup and subjective patches
On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
<Ca...@gs.com> wrote:
> Thomas, we all agree that you have done useful work. But right now it seems that you are more concerned with the aesthetics of the code base than the correctness of it. You are capable of identifying potential bugs. Are you capable of doing the work to provably identify them (by writing tests) and fix them?
Thomas has found some bugs - he's reported a few against multi support
for example. Perhaps he didn't feel enabled to work towards addressing
those.
Camille your point is a good one - the community is saying "we are
interested in addressing X", while Thomas has been addressing Y. If he
is interested to work on X I suspect he'll find broad support, if he
is only interested in Y I suspect it will continue to be an uphill
effort. My own opinion only, but I believe once X was addressed I'm
pretty sure people would be more open to doing this other work.
Q: Are we as a community clearly defining the work we are interested
in taking on in the near future?
Patrick
Re: cleanup and subjective patches
Posted by Patrick Hunt <ph...@apache.org>.
On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
<Ca...@gs.com> wrote:
> Thomas, we all agree that you have done useful work. But right now it seems that you are more concerned with the aesthetics of the code base than the correctness of it. You are capable of identifying potential bugs. Are you capable of doing the work to provably identify them (by writing tests) and fix them?
Thomas has found some bugs - he's reported a few against multi support
for example. Perhaps he didn't feel enabled to work towards addressing
those.
Camille your point is a good one - the community is saying "we are
interested in addressing X", while Thomas has been addressing Y. If he
is interested to work on X I suspect he'll find broad support, if he
is only interested in Y I suspect it will continue to be an uphill
effort. My own opinion only, but I believe once X was addressed I'm
pretty sure people would be more open to doing this other work.
Q: Are we as a community clearly defining the work we are interested
in taking on in the near future?
Patrick
Re: cleanup and subjective patches
Posted by Patrick Hunt <ph...@apache.org>.
On Tue, Nov 1, 2011 at 11:31 AM, Fournier, Camille F.
<Ca...@gs.com> wrote:
> Sorry, you're right, let me rephrase: We shouldn't be checking in code that is claiming to fix bugs without tests. I also personally think that we should strongly encourage refactorings to come in with additional tests, as per Ted's suggestion. Leave the code base better than when you left it in a tangible way. There are two patches checked in that Thomas claims are bugfixes, that came in with no tests to verify that the bug they were fixing is actually fixed and had no test in place that was failing due to the bug.
I agree. I committed those - I'll be more diligent in future.
(although 1247 looks more like a cleanup than a specific fix)
Patrick
> -----Original Message-----
> From: Patrick Hunt [mailto:phunt@apache.org]
> Sent: Tuesday, November 01, 2011 1:15 PM
> To: dev@zookeeper.apache.org
> Cc: thomas@koch.ro; Benjamin Reed
> Subject: Re: cleanup and subjective patches
>
> On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
> <Ca...@gs.com> wrote:
>> Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
>
> There's a difference btw checking in refactorings and fixing warnings
> and such that are covered by existing test, vs committing feature
> changes and bug fixes that are not caught by existing tests.
>
> Patrick
>
RE: cleanup and subjective patches
Posted by "Fournier, Camille F." <Ca...@gs.com>.
Sorry, you're right, let me rephrase: We shouldn't be checking in code that is claiming to fix bugs without tests. I also personally think that we should strongly encourage refactorings to come in with additional tests, as per Ted's suggestion. Leave the code base better than when you left it in a tangible way. There are two patches checked in that Thomas claims are bugfixes, that came in with no tests to verify that the bug they were fixing is actually fixed and had no test in place that was failing due to the bug.
C
-----Original Message-----
From: Patrick Hunt [mailto:phunt@apache.org]
Sent: Tuesday, November 01, 2011 1:15 PM
To: dev@zookeeper.apache.org
Cc: thomas@koch.ro; Benjamin Reed
Subject: Re: cleanup and subjective patches
On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
<Ca...@gs.com> wrote:
> Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
There's a difference btw checking in refactorings and fixing warnings
and such that are covered by existing test, vs committing feature
changes and bug fixes that are not caught by existing tests.
Patrick
Re: cleanup and subjective patches
Posted by Patrick Hunt <ph...@apache.org>.
On Tue, Nov 1, 2011 at 9:51 AM, Fournier, Camille F.
<Ca...@gs.com> wrote:
> Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
There's a difference btw checking in refactorings and fixing warnings
and such that are covered by existing test, vs committing feature
changes and bug fixes that are not caught by existing tests.
Patrick
RE: cleanup and subjective patches
Posted by "Fournier, Camille F." <Ca...@gs.com>.
Thomas, we all agree that you have done useful work. But right now it seems that you are more concerned with the aesthetics of the code base than the correctness of it. You are capable of identifying potential bugs. Are you capable of doing the work to provably identify them (by writing tests) and fix them?
Now that you've drawn my attention to this, I have some questions. How the heck are ZOOKEEPER-1265 and ZOOKEEPER-1247 "bugfixes" if you fixed them without putting in a single test? That is not a bugfix. Nothing about those changes precluded writing tests for them, or modifying existing tests to prove the fix. It seems like perhaps they were viewed as refactoring, because otherwise I can't imagine why they weren't tested. You don't fix a bug unless you have a test that shows the bug by failing before the change and passing afterwards. Period. Please go back and write tests for these two fixes, otherwise I may revert the checkins and reopen the tickets.
Committers, this checking in of code without tests has got to stop. It's actively detrimental to the code base and undermines any value we might be getting from refactoring efforts. With very limited exceptions (changing socket parameters or something else that is virtually impossible to test without significant investment in additional libraries and scaffolding), we should not put in ANY more changes without tests. There's a reason we see a -1 in the Hudson build report for no new tests. Don't take it lightly.
C
-----Original Message-----
From: Thomas Koch [mailto:thomas@koch.ro]
Sent: Tuesday, November 01, 2011 12:12 PM
To: dev@zookeeper.apache.org
Cc: Fournier, Camille F. [Tech]; 'Benjamin Reed'
Subject: Re: cleanup and subjective patches
Fournier, Camille F.:
> Any changes coming in without tests should really be meaningfully
> untestable. I completely agree with the suggestion to require a testing
> uplift if you want to add refactorings unless you know the refactored code
> has 90+% test coverage.
>
> Personally, I have no problems with refactorings, but we seem to be
> spending much of our time dealing with them right now when we really need
> to get 3.4 out the door. It's been months that this release has been going
> on and splitting the committer attention between 3.4 and refactoring
> changes feels counterproductive to the needs of the community at this
> moment.
>
> C
Some facts for the last two months:
25 patches submitted by me were committed
added lines: 1805
deleted lines: 5795
=====
-3990
Two patches were about big legacy code removals, excluding them:
added lines: 1770
deleted lines: 2013
=====
-243
Critical blocker bugs found because of doing refactorings:
ZOOKEEPER-1269 Multi deserialization issues
ZOOKEEPER-1246 Dead code in PrepRequestProcessor catch Exception block
And two other possible issues described in mail
"Possible failure scenarios with deserialization + multi?"
minor bugs found:
ZOOKEEPER-1247 dead code in PrepRequestProcessor.pRequest multi case
ZOOKEEPER-1272 ZooKeeper.multi() could violate API if server misbehaves
ZOOKEEPER-1265 Normalize switch cases lists on request types
functional improvements:
ZOOKEEPER-1175 DataNode references parent node for no reason
ZOOKEEPER-556 Startup messages should account for common error of missing
leading slash in config files
missing tests added:
ZOOKEEPER-1254 test correct watch handling with multi ops
ZOOKEEPER-1259_central_mapping_from_type_to_txn_record_class (not yet
committed)
At the beginning of October I also reviewed _all_ open bugs and closed a
couple of obsolete onces or asked for feedback.
Regards,
Thomas Koch, http://www.koch.ro
Re: cleanup and subjective patches
Posted by Thomas Koch <th...@koch.ro>.
Fournier, Camille F.:
> Any changes coming in without tests should really be meaningfully
> untestable. I completely agree with the suggestion to require a testing
> uplift if you want to add refactorings unless you know the refactored code
> has 90+% test coverage.
>
> Personally, I have no problems with refactorings, but we seem to be
> spending much of our time dealing with them right now when we really need
> to get 3.4 out the door. It's been months that this release has been going
> on and splitting the committer attention between 3.4 and refactoring
> changes feels counterproductive to the needs of the community at this
> moment.
>
> C
Some facts for the last two months:
25 patches submitted by me were committed
added lines: 1805
deleted lines: 5795
=====
-3990
Two patches were about big legacy code removals, excluding them:
added lines: 1770
deleted lines: 2013
=====
-243
Critical blocker bugs found because of doing refactorings:
ZOOKEEPER-1269 Multi deserialization issues
ZOOKEEPER-1246 Dead code in PrepRequestProcessor catch Exception block
And two other possible issues described in mail
"Possible failure scenarios with deserialization + multi?"
minor bugs found:
ZOOKEEPER-1247 dead code in PrepRequestProcessor.pRequest multi case
ZOOKEEPER-1272 ZooKeeper.multi() could violate API if server misbehaves
ZOOKEEPER-1265 Normalize switch cases lists on request types
functional improvements:
ZOOKEEPER-1175 DataNode references parent node for no reason
ZOOKEEPER-556 Startup messages should account for common error of missing
leading slash in config files
missing tests added:
ZOOKEEPER-1254 test correct watch handling with multi ops
ZOOKEEPER-1259_central_mapping_from_type_to_txn_record_class (not yet
committed)
At the beginning of October I also reviewed _all_ open bugs and closed a
couple of obsolete onces or asked for feedback.
Regards,
Thomas Koch, http://www.koch.ro
Re: cleanup and subjective patches
Posted by Patrick Hunt <ph...@apache.org>.
On Tue, Nov 1, 2011 at 7:21 AM, Fournier, Camille F.
<Ca...@gs.com> wrote:
> Personally, I have no problems with refactorings, but we seem to be spending much of our time dealing with them right now when we really need to get 3.4 out the door. It's been months that this release has been going on and splitting the committer attention between 3.4 and refactoring changes feels counterproductive to the needs of the community at this moment.
I second that. There are a few 3.4 blocker issues pending that need
review -- not to mention Mahadev hasn't gotten much feedback on the
release candidate.
There are 35 patch available issues at this moment, most of which are
_not_ refactoring issues. Plenty of patches for committers to review
if they are uninterested in aesthetic issues.
Patrick
>
> -----Original Message-----
> From: Ted Dunning [mailto:ted.dunning@gmail.com]
> Sent: Monday, October 31, 2011 7:08 PM
> To: dev@zookeeper.apache.org
> Cc: thomas@koch.ro; Benjamin Reed
> Subject: Re: cleanup and subjective patches
>
> How about a middle ground being that aesthetic changes will only be
> considered if they bring significant new testing that helps mitigate the
> stability risk?
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <ph...@apache.org> wrote:
>
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <te...@gmail.com>
>> wrote:
>> > Jumping over to Ben's side of the discussion,
>> >
>> > Git helps with this, but does not eliminate the problem. At some point
>> the
>> > changes become difficult to understand relative to the new code and may
>> > even collide in ways that are difficult to merge.
>> >
>> > It is true, however, that patches can be kept live using tools like git.
>> > That is how I kept the multi stuff alive, but there was at least one
>> > tricky merge to be done.
>> >
>> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <th...@koch.ro> wrote:
>> >
>> >> Thanks to Ted for replying. I will save the mail I started in the drafts
>> >> folder until I've calmed down again.
>> >>
>> >> Benjamin Reed:
>> >> > deprioritizing them doesn't help because the patches themselves bit
>> >> > rot. shortly they will not apply and then they will be worthless. the
>> >> > poor contributor would then be left with the task of maintaining a
>> >> > patch that would never commit.
>> >> You should really give GIT a try. I've kept a pipeline of half a douzend
>> >> patches filled and current over the last two months while drinking my
>> >> morning
>> >> coffee.
>> >> Likewise have I updated my development branch of over a douzend commits
>> >> every
>> >> morning against the new ZK trunk.
>> >>
>> >> Regards,
>> >>
>> >> Thomas Koch, http://www.koch.ro
>> >>
>> >
>>
>
RE: cleanup and subjective patches
Posted by "Fournier, Camille F." <Ca...@gs.com>.
Any changes coming in without tests should really be meaningfully untestable. I completely agree with the suggestion to require a testing uplift if you want to add refactorings unless you know the refactored code has 90+% test coverage.
Personally, I have no problems with refactorings, but we seem to be spending much of our time dealing with them right now when we really need to get 3.4 out the door. It's been months that this release has been going on and splitting the committer attention between 3.4 and refactoring changes feels counterproductive to the needs of the community at this moment.
C
-----Original Message-----
From: Ted Dunning [mailto:ted.dunning@gmail.com]
Sent: Monday, October 31, 2011 7:08 PM
To: dev@zookeeper.apache.org
Cc: thomas@koch.ro; Benjamin Reed
Subject: Re: cleanup and subjective patches
How about a middle ground being that aesthetic changes will only be
considered if they bring significant new testing that helps mitigate the
stability risk?
On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <ph...@apache.org> wrote:
> EOD this is not a tool problem, it's a resource issue. We have limited
> committer time available for reviews. IMO Thomas's changes have been
> pretty mechanical so far, they are easy to review/commit. They do
> result in some patch churn but I've been surprised it's been so little
> so far and the code is improving as a result. However we've just
> scratched the surface (the easy stuff), I do have some concerns as we
> make more significant structural changes. Personally I think we need
> to focus on beefing up testing prior to making more significant
> changes.
>
> Ben - We won't get new committers if we limit contributions. We'll
> just dig a deeper hole for ourselves. Granted if we only review/accept
> patches of a particular type (or from a particular person) we'll be in
> a similar situation.
>
> Thomas - Ben has highlighted some good points, if we don't discuss
> these rationally, out in the open, we won't be able to make progress.
> There are other patches from other contributors that we need to
> balance the available resources against.
>
> http://communityovercode.com/over/
>
>
> Personally: my biggest concern is that we keep releasing solid high
> quality code that works in production. I'm seeing patches go in that
> break existing functionality and no one notices. To me the thing we
> should be focusing on is adding testing. Refactoring and such to
> improve/increase testing imo is a net positive.
>
> Patrick
>
> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <te...@gmail.com>
> wrote:
> > Jumping over to Ben's side of the discussion,
> >
> > Git helps with this, but does not eliminate the problem. At some point
> the
> > changes become difficult to understand relative to the new code and may
> > even collide in ways that are difficult to merge.
> >
> > It is true, however, that patches can be kept live using tools like git.
> > That is how I kept the multi stuff alive, but there was at least one
> > tricky merge to be done.
> >
> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <th...@koch.ro> wrote:
> >
> >> Thanks to Ted for replying. I will save the mail I started in the drafts
> >> folder until I've calmed down again.
> >>
> >> Benjamin Reed:
> >> > deprioritizing them doesn't help because the patches themselves bit
> >> > rot. shortly they will not apply and then they will be worthless. the
> >> > poor contributor would then be left with the task of maintaining a
> >> > patch that would never commit.
> >> You should really give GIT a try. I've kept a pipeline of half a douzend
> >> patches filled and current over the last two months while drinking my
> >> morning
> >> coffee.
> >> Likewise have I updated my development branch of over a douzend commits
> >> every
> >> morning against the new ZK trunk.
> >>
> >> Regards,
> >>
> >> Thomas Koch, http://www.koch.ro
> >>
> >
>
Re: cleanup and subjective patches
Posted by Ted Dunning <te...@gmail.com>.
Thomas,
Let me second that suggestion. If you could figure out how to make tests
easier to do without starting a full scale server, it would make a BIG
positive impact.
On Mon, Oct 31, 2011 at 4:31 PM, Patrick Hunt <ph...@apache.org> wrote:
> I'd love it if someone were willing to work with me on the testing.
> Thomas? That would provide a strong foundation for future
> refactorings.
>
Re: cleanup and subjective patches
Posted by Patrick Hunt <ph...@apache.org>.
What is an aesthetic change? Thomas just removed a duplicate test in
the source base (copy/paste), should that go in or not? (imo it
should) Many of the changes that have been going in are improvements,
such as reducing warnings in eclipse so that when you are using
eclipse you can actually see "real" warnings. etc.... I've pushed back
on a few patches that were more aesthetic imo, but not many fell into
that category. Who knows if my sense of style is the same as yours...
etc...
The problem with demanding more tests is that we need to improve the
test framework itself for that to be successful. Right now most of our
tests are not unit tests. It's hard to test at the unit level. With
each release I've tried to refactor/improve the test framework, but
it's still not up to par.
I'd love it if someone were willing to work with me on the testing.
Thomas? That would provide a strong foundation for future
refactorings.
Patrick
On Mon, Oct 31, 2011 at 4:08 PM, Ted Dunning <te...@gmail.com> wrote:
> How about a middle ground being that aesthetic changes will only be
> considered if they bring significant new testing that helps mitigate the
> stability risk?
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <ph...@apache.org> wrote:
>
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <te...@gmail.com>
>> wrote:
>> > Jumping over to Ben's side of the discussion,
>> >
>> > Git helps with this, but does not eliminate the problem. At some point
>> the
>> > changes become difficult to understand relative to the new code and may
>> > even collide in ways that are difficult to merge.
>> >
>> > It is true, however, that patches can be kept live using tools like git.
>> > That is how I kept the multi stuff alive, but there was at least one
>> > tricky merge to be done.
>> >
>> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <th...@koch.ro> wrote:
>> >
>> >> Thanks to Ted for replying. I will save the mail I started in the drafts
>> >> folder until I've calmed down again.
>> >>
>> >> Benjamin Reed:
>> >> > deprioritizing them doesn't help because the patches themselves bit
>> >> > rot. shortly they will not apply and then they will be worthless. the
>> >> > poor contributor would then be left with the task of maintaining a
>> >> > patch that would never commit.
>> >> You should really give GIT a try. I've kept a pipeline of half a douzend
>> >> patches filled and current over the last two months while drinking my
>> >> morning
>> >> coffee.
>> >> Likewise have I updated my development branch of over a douzend commits
>> >> every
>> >> morning against the new ZK trunk.
>> >>
>> >> Regards,
>> >>
>> >> Thomas Koch, http://www.koch.ro
>> >>
>> >
>>
>
Re: cleanup and subjective patches
Posted by Ted Dunning <te...@gmail.com>.
But would a combined test + aesthetic patch fly in your mind?
On Mon, Oct 31, 2011 at 4:28 PM, Benjamin Reed <br...@apache.org> wrote:
> that isn't really middle ground. adding new tests for code is added
> functionality, so it would be a valid patch under the criteria i
> suggested. changing an int to an enum is not adding functionality. the
> later may be subjectively better and the former adds functionality.
> the nice thing is that you can determine that objectively. :)
>
> ben
>
> On Mon, Oct 31, 2011 at 4:08 PM, Ted Dunning <te...@gmail.com>
> wrote:
> > How about a middle ground being that aesthetic changes will only be
> > considered if they bring significant new testing that helps mitigate the
> > stability risk?
> >
> > On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <ph...@apache.org> wrote:
> >>
> >> EOD this is not a tool problem, it's a resource issue. We have limited
> >> committer time available for reviews. IMO Thomas's changes have been
> >> pretty mechanical so far, they are easy to review/commit. They do
> >> result in some patch churn but I've been surprised it's been so little
> >> so far and the code is improving as a result. However we've just
> >> scratched the surface (the easy stuff), I do have some concerns as we
> >> make more significant structural changes. Personally I think we need
> >> to focus on beefing up testing prior to making more significant
> >> changes.
> >>
> >> Ben - We won't get new committers if we limit contributions. We'll
> >> just dig a deeper hole for ourselves. Granted if we only review/accept
> >> patches of a particular type (or from a particular person) we'll be in
> >> a similar situation.
> >>
> >> Thomas - Ben has highlighted some good points, if we don't discuss
> >> these rationally, out in the open, we won't be able to make progress.
> >> There are other patches from other contributors that we need to
> >> balance the available resources against.
> >>
> >> http://communityovercode.com/over/
> >>
> >>
> >> Personally: my biggest concern is that we keep releasing solid high
> >> quality code that works in production. I'm seeing patches go in that
> >> break existing functionality and no one notices. To me the thing we
> >> should be focusing on is adding testing. Refactoring and such to
> >> improve/increase testing imo is a net positive.
> >>
> >> Patrick
> >>
> >> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <te...@gmail.com>
> >> wrote:
> >> > Jumping over to Ben's side of the discussion,
> >> >
> >> > Git helps with this, but does not eliminate the problem. At some
> point
> >> > the
> >> > changes become difficult to understand relative to the new code and
> may
> >> > even collide in ways that are difficult to merge.
> >> >
> >> > It is true, however, that patches can be kept live using tools like
> git.
> >> > That is how I kept the multi stuff alive, but there was at least one
> >> > tricky merge to be done.
> >> >
> >> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <th...@koch.ro> wrote:
> >> >
> >> >> Thanks to Ted for replying. I will save the mail I started in the
> >> >> drafts
> >> >> folder until I've calmed down again.
> >> >>
> >> >> Benjamin Reed:
> >> >> > deprioritizing them doesn't help because the patches themselves bit
> >> >> > rot. shortly they will not apply and then they will be worthless.
> the
> >> >> > poor contributor would then be left with the task of maintaining a
> >> >> > patch that would never commit.
> >> >> You should really give GIT a try. I've kept a pipeline of half a
> >> >> douzend
> >> >> patches filled and current over the last two months while drinking my
> >> >> morning
> >> >> coffee.
> >> >> Likewise have I updated my development branch of over a douzend
> commits
> >> >> every
> >> >> morning against the new ZK trunk.
> >> >>
> >> >> Regards,
> >> >>
> >> >> Thomas Koch, http://www.koch.ro
> >> >>
> >> >
> >
> >
>
Re: cleanup and subjective patches
Posted by Benjamin Reed <br...@apache.org>.
that isn't really middle ground. adding new tests for code is added
functionality, so it would be a valid patch under the criteria i
suggested. changing an int to an enum is not adding functionality. the
later may be subjectively better and the former adds functionality.
the nice thing is that you can determine that objectively. :)
ben
On Mon, Oct 31, 2011 at 4:08 PM, Ted Dunning <te...@gmail.com> wrote:
> How about a middle ground being that aesthetic changes will only be
> considered if they bring significant new testing that helps mitigate the
> stability risk?
>
> On Mon, Oct 31, 2011 at 3:51 PM, Patrick Hunt <ph...@apache.org> wrote:
>>
>> EOD this is not a tool problem, it's a resource issue. We have limited
>> committer time available for reviews. IMO Thomas's changes have been
>> pretty mechanical so far, they are easy to review/commit. They do
>> result in some patch churn but I've been surprised it's been so little
>> so far and the code is improving as a result. However we've just
>> scratched the surface (the easy stuff), I do have some concerns as we
>> make more significant structural changes. Personally I think we need
>> to focus on beefing up testing prior to making more significant
>> changes.
>>
>> Ben - We won't get new committers if we limit contributions. We'll
>> just dig a deeper hole for ourselves. Granted if we only review/accept
>> patches of a particular type (or from a particular person) we'll be in
>> a similar situation.
>>
>> Thomas - Ben has highlighted some good points, if we don't discuss
>> these rationally, out in the open, we won't be able to make progress.
>> There are other patches from other contributors that we need to
>> balance the available resources against.
>>
>> http://communityovercode.com/over/
>>
>>
>> Personally: my biggest concern is that we keep releasing solid high
>> quality code that works in production. I'm seeing patches go in that
>> break existing functionality and no one notices. To me the thing we
>> should be focusing on is adding testing. Refactoring and such to
>> improve/increase testing imo is a net positive.
>>
>> Patrick
>>
>> On Mon, Oct 31, 2011 at 2:39 PM, Ted Dunning <te...@gmail.com>
>> wrote:
>> > Jumping over to Ben's side of the discussion,
>> >
>> > Git helps with this, but does not eliminate the problem. At some point
>> > the
>> > changes become difficult to understand relative to the new code and may
>> > even collide in ways that are difficult to merge.
>> >
>> > It is true, however, that patches can be kept live using tools like git.
>> > That is how I kept the multi stuff alive, but there was at least one
>> > tricky merge to be done.
>> >
>> > On Mon, Oct 31, 2011 at 2:35 PM, Thomas Koch <th...@koch.ro> wrote:
>> >
>> >> Thanks to Ted for replying. I will save the mail I started in the
>> >> drafts
>> >> folder until I've calmed down again.
>> >>
>> >> Benjamin Reed:
>> >> > deprioritizing them doesn't help because the patches themselves bit
>> >> > rot. shortly they will not apply and then they will be worthless. the
>> >> > poor contributor would then be left with the task of maintaining a
>> >> > patch that would never commit.
>> >> You should really give GIT a try. I've kept a pipeline of half a
>> >> douzend
>> >> patches filled and current over the last two months while drinking my
>> >> morning
>> >> coffee.
>> >> Likewise have I updated my development branch of over a douzend commits
>> >> every
>> >> morning against the new ZK trunk.
>> >>
>> >> Regards,
>> >>
>> >> Thomas Koch, http://www.koch.ro
>> >>
>> >
>
>