Re: Improvements of master

4 views
Skip to first unread message

Nils Kübler

unread,
Nov 4, 2013, 3:14:46 AM11/4/13
to Vitaliy Morarian, hannib...@googlegroups.com
Hi Vitaliy,

Wonderful! I agree with most parts. However I wonder why the Futures should be removed. As far as I understood the Playframework, it can only handle one request at once unless you’re returning a future, thats why we added all those futures. http://www.playframework.com/documentation/2.0.x/ScalaAsync 

Thank you, please keep on with the good work!

Nils


--
Nils Kübler
Software Engineer
____________________________________

YMC AG
Sonnenstr. 4
CH-8280 Kreuzlingen
Switzerland

Tel +41 (0)71 / 508 24 81
Fax +41 (0)71 / 560 53 89

Web www.ymc.ch
____________________________________ 


On 31. Oktoober 2013 at 20:32:59, Vitaliy Morarian (vmor...@gmail.com) wrote:

Hi Nils,

I slightly refactored master branch
https://github.com/vmorarian/hannibal/tree/refactoring

I removed all Future's because in this case it doesn't give any performance. It would help when more than 1 future is created/map'ed.
Also in old code it was used await blocking method.

Removed mutable structures.

Take please also a look, that:

RegionName case class has overridden equals but hashCode is absent

It's not good idea to create case class which has mutable data 
case class Region(val regionServer: RegionServer, val regionLoad: HServerLoad.RegionLoad) 
Other issues is that in this case we can't use convenient method copy (because all vals are fields)

Let me know what you think about it. Should I continue with it?

With best regards,
Vitaliy Morarian

Nils Kübler

unread,
Nov 5, 2013, 5:21:21 AM11/5/13
to Vitaliy Morarian, hannib...@googlegroups.com
Hi Vitaliy,

Thank you for the clarification. We were just following the guidelines and the way it’s currently implemented in Hannibal is as described in play 2.0 documentation: 
val promiseOfInt: Promise[Int] = Akka.future {
  intensiveComputation()
}
We thought that this would be correct, at least for version 2.0 . Since version 2.1, as far as I understand, we would need to configure and import an ExecutionContext for this to make sense.

I already tried to port of Hannibal to 2.1 in the past, but after spending hours I did cancel it as it was a huge hassle to port the code to the new JSON APIs. I would really appreciate a 2.2 port! I would be very glad if someone would work on that :) 

I agree with the way to use futures in controllers instead of the models. However I am fine with your suggestion to just remove them. This should simplify things. We could add them anytime again, if required. 


Best regards,
Nils Kübler


--
Nils Kübler
Software Engineer
____________________________________

YMC AG
Sonnenstr. 4
CH-8280 Kreuzlingen
Switzerland

Tel +41 (0)71 / 508 24 81
Fax +41 (0)71 / 560 53 89

Web www.ymc.ch
____________________________________ 


On 4. Novämber 2013 at 16:39:28, Vitaliy Morarian (vmor...@gmail.com) wrote:

ould like to c
Reply all
Reply to author
Forward
0 new messages