Re: [OJB-developers] RequestProcessor reworked ...

[Thomas M. <tho...@ho...>]

Hy again Joachim,

Joa...@tp... wrote:

<snip executive summary>
 
> --- begin gory details block ---
> right now I'm deeply into the C/S-model of ObjectBridge.


welcome to the club of hardcore ojb hackers !

> But I'm having a
> problem. The JUnit tests worked well in singlevm mode on mysql (except for
> the one Test that failes 'cause MySQL handles CLOBs in a non-standard way),
> but as soon as I started it in Client/Server mode it started to act up.


Can you imagine all the pain I suffered to get this stuff running for 
the first time?


> 
> First of all I searched 2 hours for an error until I noticed that my
> experimental support for global UIDs in SequenceManagerHighLowImpl (which


A side note: OJB provides a GUID class in ojb.broker.util.GUID. Maybe you find it useful.

> was a very, very ugly hack) wreaked havoc, but after this was fixed I still
> had problems. After some further investigation (and some prayer that the
> cause-exception handling of 1.4 will become more common) I found out that
> the server choked on .store() outside of transactions (which I think is
> legal, at least in the JUnit tests in singlevm mode it works).
> 
> The problem was the code
> 
>             broker.store(arg1, (ObjectModification) arg2);
>             ((PersistenceBrokerImpl) broker).checkPoint();
> 
> i changed it to
> 
>             broker.store(arg1, (ObjectModification) arg2);
>             if (broker.isInTransaction()) {
>                 ((PersistenceBrokerImpl) broker).checkPoint();
>             }
> 
> which at least solved the symptoms. Is the checkPoint really necessary?
> 


Oops. I thought I kicked out the checkpoint stuff already. I think it's 
pretty superfluous.


> Later on I found one more problem: When the Client executed a
> beginTransaction after already haveing done some talking with the server
> (outside of an Transaction) the Server chocked, 'cause it thought that the
> fact that the client already got an brokerId automatically ment that it
> already had a running transaction (which is wrong, AFAICS).


Mmh, my initial idea was to only assign an id once a tx was started. As 
this is the only reason why a client should be served by the same broker 
instance. I don't see why we should assign a brokerid (which is 
something like a session cookie) outside a tx.


> 
> After I got the default C/S-System to run I refactored the RequestProcessor
> to take out the Socket-Handling code to allow me to reuse it for my
> soon-to-come PersistenceBrokerServlet.


That's a very good refactoring !

> The Socket Handling Code became a
> new class ConnectionHandler (I'd have made it a private inner class of
> PersistenceBrokerServer, but I know that most other ppl don't like this
> very much).



As you will have noticed, I did not use inner classes. (Jakob uses them 
in certain places). I don't use inner classes as some IDE's don't 
provide good support for visualizing inner classes. But I guess it's OK 
to have inner classes for certain details.


> 
>  Then I started refactoring dispatch(). Most of the methods like store(),
> open(), getUniqeId() have only two lines difference (the common code beeing
> the brokerId-is-still--1-code and the exception handling). I moved both the
> brokerId-handling code and the exception handlig out of the specific
> functions and as the remaining functions all had only one or two lines left
> I inlined them into the call, saving some hundered lines of codes. 


this is a very good refactoring too!

>I know
> this is a matter of taste, but I prefer it this way, I'd like to hear your
> oppinion on this one.
> 


IMHO this was really a "smelly" piece of code !


> Basically the major improvement (IMHO) was that I changed the broker
> allocation scheme. Now if the client provides a brokerId this broker is
> always used (which opens a huge security hole, but this was not different
> before). 


I did not bother with security issues in my initial design.
The brokerId is only a kind of "cookie" to identify a client transaction.
To make this secure we "simply" have to ensure that only the server can 
generate such transaction IDs and that the client can't produce fake 
ID's to contact brokers serving other transactions.
(This is left as an exercise to the interested reader ;-))

>At the end of the-function-formerly-known-as-dispatched (which I
> renamed to execute, to reflect its new functionality) the broker is
> released, unless it has a running transaction (which only happens, when the
> client has requested a beginTransaction). This seems to work fairly well.


Yes that should be OK!


> At least I now pass all JUnit Test while in Client/Server mode (except of
> course PersistenceBrokerTest.testEscaping which MySQL can't pass).
> 


Did you're changes also pass the ODMG tests? They contain some special 
tests not checked in the broker tests.


> before I forget it: one functionality was lost and I don't know yet what to
> do about it: srvPing now no longer can use  connection.getLocalAddress() as
> it is handled in RequestProcessor which no longer knows about the socket.
> 


This can be replaced by InetAddress.getLocalHost().getHostAddress().


> Unfortunately I can't make a reasonable diff, due to the newline-problem in
> CVS (it allways produces a diff that is twice the size of the files to be
> diffed), so I'll just include a .zip with the two changed files
> (PersistenceBrokerServer and RequestProcessor) and the new File
> (ConnectionHandler).


Joachim, I guess it would be much simpler if you check in your code 
directly into the CVS archive. What do you think?
If you want to have developer access to the CVS repository please tell 
me your SourceForge user name and I will setup everything for you!

Of course if you prefer to send in your code via email this is also OK 
for me.

thanks for your efforts !

Thomas


> --- end gory details block ---
> 
> btw, shouldn't static finals (as in PBMethodIndex) be written in
> all-uppercase (i.e. ABORT_TRANSACTION instead of abortTransaction)?
> 


Yes, it would look much more professional that way ;-)