cayenne-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dzmitry Kazimirchyk <dkazimirc...@gmail.com>
Subject Re: work on ROP
Date Thu, 21 Jan 2016 12:39:37 GMT
On 1/21/16 1:15 PM, Aristedes Maniatis wrote:
> On 21/01/2016 3:35am, Dzmitry Kazimirchyk wrote:
>> At this point the implementation is quite crude and lacks proper unit tests, but
it shows the general direction we could follow. Comments and suggestions are welcome.
>
> I think it looks really clear. I'm going to try and assemble a bunch of questions, some
small and some big, to help my understanding. And some is just about code which was there
from before.
>
>
> BaseConnection.sendMessage(message) does pretty much nothing but call beforeSendMessage(message),
then doSendMessage(message) with a bunch of logging. It all seems a bit pointless.
>

True. Before my changes it also used to store readTimeout value, but now 
all it does is logging, which can be moved a level down to the actual 
ClientConnection implementation.

> ----
>
> ROPConnector could do with some docs to explain the difference between a sharedSession
and a Session.

It sure could :). I will add docs shortly, for now I just wanted to 
flesh out the working concept.

>
> ----
>
> So DefaultClientConnectionProvider is what creates a ProxyRemoteService and passes to
it the serializationService and the ropConnector. Why is the serializationService injected
here but the ropConnector hardcoded to HttpROPConnector?
>
> Doesn't that effectively make this a HttpClientConnectionProvider rather than a DefaultClientConnectionProvider?
Should DefaultClientConnectionProvider.createHttpRopConnector() be removed?
>
> ----
>
> For that matter, do we need quite so many layers? Could the functionality of ProxyRemoteService
just be brought into DefaultClientConnectionProvider? Maybe I'm just not quite understanding
the need for both of these things separately.
>

Well, my first idea was to make all three of RemoteService, ROPConnector 
and ClientConnection interfaces completely independent from each other. 
But it in reality HttpROPConnector needs to have access to RemoteSession 
object from ClientConnection to add appropriate session header to HTTP 
request it makes. Hence all that logic in 
DefaultClientConnectionProvider effectively tying it with 
HttpROPConnector implementation.

> ------
>
> HttpROPConnector might do with some way to set the cookie name. You made SESSION_COOKIE_NAME
final for some reason.

SESSION_COOKIE_NAME is just the name of the HTTP session header, but 
having some sort of pipeline requests go through to be able to add 
various parameters like compression, custom cookies, etc. might be 
beneficial.

>
> -----
>
> Instead of reinventing HttpROPConnector.base64(), can we use this: https://docs.oracle.com/javase/8/docs/api/java/util/Base64.html
>

Currently Cayenne needs to support Java 7 which doesn't have Base64 class.

One of the questions here is still whether we want to go all the way and 
introduce changes which can potentially brake things for upgrading users 
or we should preserve the old HessianROPServlet and the logic around it 
and have a new pluggable approach as an alternative to what we currently 
have.


Dima



Mime
View raw message