2014-03-07 23:16 GMT+01:00 Konstantin Kolinko <knst.koli...@gmail.com>:
> 1. It is a month since release 8.0.3 and thus I think 8.0.4 is > expected in a week or so.. > > I am -1 to destabilize 8.0.x now. > As a new component, it is not supposed to destabilize the other components that are in 8.0. > > To this, as you are saying that some tests do not pass. I woudn't want > to make buildbot fail this close to a release, as we may miss > something important. > It will cause failures only if the tests are enabled for this connector at build time, there's no reason to do so at the moment since it is experimental. > > So I think the time to merge this is not earlier than 8.0.4 is voted > and released + some time to cool off. I think that is about 10 days > from now. > > 2. How am I supposed to review this? > Is there some viewable "patch" against trunk? > You just gave the link for the commit below, it shows all possible diffs, and it is rebased against trunk. > > The comments below are from a quick review of > > > https://github.com/rmaucher/tomcat/commit/614d8c43d8d1f3eeb4d5e4c2493ead589a72bf2c > > 1) There are a number of unrelated changes, including some comment / > typo fixes. That does not help reviewing. Why aren't those in Tomcat > trunk yet? > There's only a very small amount of them (maybe 3), it shouldn't hinder reviewing in any way. Actually, that's a good idea, I'll go ahead and commit the changes to the existing classes, since I consider they make some sense on their own. I'm not asking for a full review anyway, just some general feedback on the component. > > 2). There is a lot of code that from the first glance seems similar to > Nio1 one. > Just a matter of preference, not a stopper. > The shared code can be move to a superclass eventually. But it is very different, a lot of major pieces of code from the NIO1 connector have no equivalent. > > (In your opening e-mail you say there are many differences. This > similarity is my first impression. Maybe there aren't any. I'd need > some time to compare). > > 3). What are the selling points of this implementation? > I described some of the pros and cons of the NIO2 API. If the pros end up in the implementation without being degraded, it means the connector should be faster, more resources friendly, simpler (no poller, no selector, etc), since that's what happened with a similar NIO2 connector in JBoss. But I can't make any big promises yet. The documentation part of the patch does not say anything in particular. > Also my -1 that it does not say that this connector is an "experimental" > one. > On startup, it logs: WARNING [main] org.apache.tomcat.util.net.Nio2Endpoint.startInternal The NIO2 connector is currently EXPERIMENTAL and should not be used in production So it is annoyingly visible, but I can add another mention in the docs. Rémy
