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
