search

vert-x / mod-lang-groovy

Vert.x 2.x is deprecated - use instead
https://github.com/vert-x3/vertx-lang-groovy
Other
16 stars 15 forks source link

Improved sendWithTimeout event bus functionality #53

Closed jkeys089 closed 10 years ago

jkeys089 commented 10 years ago

Signed-off-by: Jon Keys jon.keys@gmail.com

Enables new 'sendWithTimeout' event bus features such as reporting failures and setting a default bus timeout.

silviucm commented 10 years ago

Please note that without Jon's commit above, the existing code explodes, with

Exception in Groovy verticle org.codehaus.groovy.runtime.typehandling.GroovyCastException: Cannot cast object 'org.vertx.java.core.impl.DefaultFutureResult@ef93f0' with class 'org.vertx.java.core.impl.DefaultFutureResult' to class 'org.vertx.java.core.eventbus.Message'

The reason is that the Java implementation of sendWithTimeout uses Handler<AsyncResult<Message>> replyHandler instead of Handler<Message> replyHandler (as the regular send does)

cheers silviu

LostInBrittany commented 10 years ago

Going to test it later today, thanks @silviucm and @jkeys089 !

On Tue, Jan 28, 2014 at 5:22 AM, silviucm notifications@github.com wrote:

Please note that without Jon's commit above, the existing code explodes, with

Exception in Groovy verticle org.codehaus.groovy.runtime.typehandling.GroovyCastException: Cannot cast object 'org.vertx.java.core.impl.DefaultFutureResult@ef93f0' with class 'org.vertx.java.core.impl.DefaultFutureResult' to class 'org.vertx.java.core.eventbus.Message'

The reason is that the Java implementation of sendWithTimeout uses Handler>> replyHandler instead of Handler> replyHandler (as the regular send does)

cheers silviu

— Reply to this email directly or view it on GitHubhttps://github.com/vert-x/mod-lang-groovy/pull/53#issuecomment-33450219 .

jkeys089 commented 10 years ago

@danthegoodman I actually looked at that but it uses type coercion to implement the required interface. Unfortunately this isn't satisfied at compile time (possibly because of this @CompileStatic(TypeCheckingMode.SKIP) annotation?). During profiling of my app this runtime type coercion was identified as a hot spot.

I agree that my wrapAsyncHandler implementation isn't really idiomatic groovy but since it isn't exposed to the API I think it is OK to choose efficiency over grooviness.

danthegoodman commented 10 years ago

@jkeys089 Interesting. It looks like that can be compiled statically, so I opened #55 for it. Maybe that will fix the performance issue you saw.

On a side note, how are you profiling your app? I'm not familiar with profiling, so is there a specific tool or procedure you recommend?

LostInBrittany commented 10 years ago

So @jkeys089 and @danthegoodman , what do we do with this pull request? Do I merge it as it is, modifications are needed?

Also, could you please add some unit test? We try to have at least some unit testing for all new functionalities.

If it isn't possible, I'll merge and add an issue for the test (that I'll try to do as soon as possible).

jkeys089 commented 10 years ago

@danthegoodman Unfortunately that does NOT resolve the issue. I use YourKit Profiler and I find it VERY useful. It is relatively inexpensive for a single license and it is well worth the $$ IMHO. There is also VisualVM which is pretty good and best of all it is free.

@LostInBrittany I would like to see this fix make it into an official release soon. Wether we use the EventBus.wrapAsyncHandler method or ClosureUtil.wrapAsyncResultHandler is up to you as the maintainer I think. It would be great to have a single method handle both cases (just less code to maintain) but I don't think it is such a critical decision at this point. It could easily be refactored later if needed.

As for unit tests, the original PR came with 6 new tests which covers 100% of changes. Do you need something more?

LostInBrittany commented 10 years ago

I guess the tests cases are good at lest for now, we can always add more later.

If you think it doesn't break anybody's code, we can merge it.

Merging it later today (away from computer now). On Jan 29, 2014 3:49 PM, "jkeys089" notifications@github.com wrote:

@danthegoodman https://github.com/danthegoodman Unfortunately that does NOT resolve the issue. I use YourKit Profilerhttp://www.yourkit.com/overview/index.jspand I find it VERY useful. It is relatively inexpensive for a single license and it is well worth the $$ IMHO. There is also VisualVMhttp://visualvm.java.net/which is pretty good and best of all it is free.

@LostInBrittany https://github.com/LostInBrittany I would like to see this fix make it into an official release soon. Wether we use the EventBus.wrapAsyncHandler method or ClosureUtil.wrapAsyncResultHandler is up to you as the maintainer I think. It would be great to have a single method handle both cases (just less code to maintain) but I don't think it is such a critical decision at this point. It could easily be refactored later if needed.

As for unit tests, the original PR came with 6 new tests which covers 100% of changes. Do you need something more?

Reply to this email directly or view it on GitHubhttps://github.com/vert-x/mod-lang-groovy/pull/53#issuecomment-33590539 .

danthegoodman commented 10 years ago

@jkeys089

Unfortunately that does NOT resolve the issue.

Shoot. :disappointed:

It would be great to have a single method handle both cases (just less code to maintain) but I don't think it is such a critical decision at this point. It could easily be refactored later if needed.

I agree 100%.

I use YourKit Profiler and I find it VERY useful.

YourKit looks interesting. I'll check it out. Thanks for the tip!

I appreciate your willingness to communicate and good job on profiling. Thanks.

jkeys089 commented 10 years ago

@LostInBrittany I think the point @silviucm was making is that the sendWithTimeout methods are currently broken without these additions :-) If you feel more tests are needed I'm happy to add them. What specifically is your concern? Also, I think altering the ClosureUtil.wrapAsyncResultHandler is far more risky in terms of "breaking anybody's code" compared to adding a new method specifically for handling sendWithTimeout responses which is fully tested within this PR.

silviucm commented 10 years ago

Hi All - that's correct. My humble recommendation: commit Jon's code asap, as there's practically nothing to lose at this point, the method is currently broken, as in: if someone just arrives, downloads vert.x from master and the groovy module snapshot as well and builds and tests it, the sendWithTimeout method will not function at all.

I personally have a custom class with the sendWithTimeout that makes use of the ClosureUtil.wrapAsyncResultHandler, but I believe Jon's approach would work just as well. (I'm not the sharpest Groovy knife around, did mainly .net in the past years, so I couldn't comment on the most efficient approach)

Cheers to all ! silviu

LostInBrittany commented 10 years ago

Test ok, merge done And THANKS to all the people on this thread!