Upgrade netty to 3.2.5.Final
Martin Grotzke
currently spymemcached uses netty 3.1.5.GA. Is it possible to upgrade
netty to 3.2.5.Final?
I'm interested in this as the older netty version causes issues with
integration-tests of memcached-session-manager [1] which I want to
upgrade to spymemcached 2.7.1 for adding membase (vbucket) support.
The caused issue is a
java.lang.NoSuchMethodError:
org.jboss.netty.handler.codec.http.HttpRequest.setHeader(Ljava/lang/
String;Ljava/lang/String;)V
at
net.spy.memcached.vbucket.BucketMonitor.prepareRequest(BucketMonitor.java:
125)
which is caused by the changed signature to setHeader(String, Object) in
the newer netty version.
I just pushed the netty version in ivy/libraries.properties to
3.2.5.Final and ran tests (SPYMC_SERVER_TYPE=membase ant test) against a
running membase-server (1.7.1.1). Before (current master with 3.1.5.GA)
there were 2 failures and 5 errors, with netty 3.2.5.Final there were 2
failures and 4 errors.
The error/failure reports are attached for reference.
Appreciating your feedback,
cheers,
Martin
Mike Wiederhold
I'm okay with the idea as long as others don't say that this will
break there applications. At the moment it looks like no one really
minds. The reason I haven't make a decision yet is that I am waiting
for one of our other core contributors to get back from vacation so I
can discuss this change with him. He should be back today so we will
probably make a decision about this on Monday. We are aware of your
issue though and will follow up as soon as possible.
I also took a look at your test results and we have a few test cases
that we know to fail only sometimes. These failures are due to issues
with how the tests are written as opposed to being real issues in the
client. I also want to mention that if you are running against Membase
and you are using a version of Spymemcached that has the ivy build
file then you will want to do a "ant test -Dserver.type=membase". I'll
try to update the documentation as soon as possible so that this won't
trip up others in the future.
On Sep 22, 2:27 pm, Martin Grotzke <martin.grot...@googlemail.com>
wrote:
> < 1KViewDownload
Matt Ingenthron
Sorry for the delay in reply. I'd been out for a few days.
Do you have any idea why they changed the signature?
That's rather annoying. I'm not against it necessarily, but it seems
that if we update to a later netty, it'll force people for whom it's
currently working to see the NoSuchMethod at upgrade if they're not
using something to resolve the dependencies. :(
It also would mean some people could have a different incompatibility,
but that one is hard to evaluate.
I guess we could hack around it somehow, by going with String,Object if
we get a NoSuchMethod.
I'd feel much better doing an upgrade of netty if there were a really
good reason the method signature had changed.
Thanks for doing the work.
Matt
Martin Grotzke
On 09/27/2011 09:04 AM, Matt Ingenthron wrote:
> Hi Martin,
>
> Sorry for the delay in reply. I'd been out for a few days.
>
> Do you have any idea why they changed the signature?
They changed it due to https://issues.jboss.org/browse/NETTY-281, the
commit is https://github.com/netty/netty/commit/d0e886c3
> That's rather annoying. I'm not against it necessarily, but it seems
> that if we update to a later netty, it'll force people for whom it's
> currently working to see the NoSuchMethod at upgrade if they're not
> using something to resolve the dependencies. :(
>
> It also would mean some people could have a different incompatibility,
> but that one is hard to evaluate.
Right.
> I guess we could hack around it somehow, by going with String,Object if
> we get a NoSuchMethod.
I'd think that this won't work as the signature is bound at compilation
time. Probably one would have to select and invoke the appropriate
setHeader method via reflection.
> I'd feel much better doing an upgrade of netty if there were a really
> good reason the method signature had changed.
I totally agree. But I suspect that they won't change the signature
again, and there's also no way to add some compatibility method that
breaks nothing.
Another alternative would probably be to provide two versions of
spymemcached, with a different classifier for the older (or newer) netty
version. That would allow users, that run into an incompatibility to
pull in the other spymemcached version.
Probably this would require much more effort, so that my personal
preference would be to select and invoke setHeader via reflection.
AFAICS this change would only affect BucketMonitor.prepareRequest
(invoked from startMonitor invoked from subscribe) which should be
called not often and thus the slower performance should not be an issue.
Shall I submit a pull request for this?
Cheers,
Martin
Martin Grotzke
I just submitted a pull request for the proposed change:
https://github.com/dustin/java-memcached-client/pull/5
Cheers,
Martin
Matt Ingenthron
You are fast!
It's unfortunate that they just killed the String version of the addHeaders() method. It sure looks like they could have kept both. Ironically, this is is exactly like spymemcached issue 199.
Would you mind terribly submitting this through gerrit? It's pretty simple. After the initial setup, it's really just a matter of pushing to a different remote. This will help get others involved in the review.
The instructions are here:
http://www.couchbase.org/wiki/display/membase/Contributing+changes
Once you're signed in, I can give you the appropriate permissions. Just send me a direct email.
And thanks!
Matt