actually, the other way of doing the netty patch (since i'm scared of merges) would be to do a refactor cleanup patch with an eye toward netty, and then another patch to actually add netty. that would have been nice because the first patch would allow us to more easily make sure that NIO wasn't broken. and the second we could focus more on the netty addition.


On 10/15/2010 03:07 PM, Patrick Hunt wrote:
On Fri, Oct 15, 2010 at 12:11 PM, Henry Robinson<>  wrote:

The netty patch is a good test case for this approach. If we feel that
reworking the structure of the existing server cnxn code will make it
significantly easier to add a second implementation that adheres to the
interface, then I say that such a refactoring is worthwhile, but even then
only if it's straightforward to make the changes while convincing ourselves
that the behaviour of the new implementation is consistent with the old.

Thomas, do comment on the patch itself! That's the very best way to make
sure your concerns get heard and addressed.

Well really the _best_ way IMO is to both comment and submit a patch. ;-)

And this is just what Thomas is doing, so kudos to him for the effort!
Vishal is doing this as well for many of the issues he's found, so thanks to
him also. We do appreciate you guys jumping in to help. Lack of contributors
is one of the things we've been missing and addressing that opens the door
to some of these improvements being suggested.

Wrt the netty patch, the approach Ben and I took was to refactor
sufficiently to add support for NIO/Netty/... while minimizing breakage.
This is already a big patch, esp given that the code is not really as clean
to begin with (complex too). Perfect situation, no. But the intent was to
further clean things up once the original patch was reviewed/committed.
Trying to do a huge refactoring in one shot (one patch) is not a good idea
imo. Already these patches are too large. Perhaps lesson learned here is
that we should have just created a special branch from the get go, applied a
number of smaller patches to that branch, then eventually merged back into
the trunk once it was fully baked.


Reply via email to