jackrabbit-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Esteban Franqueiro" <esteban.franque...@bea.com>
Subject Re: [jira] Commented: (JCR-1154) Database Data Store
Date Thu, 01 Nov 2007 19:27:19 GMT
Hi Thomas.

> - please use spaces, not tabs (Jackrabbit consistently uses spaces)
> - please don't use import ...*
> - byte b[] > byte[] b
> - public is not required in interfaces
> - return does not require (..)
>
> To fix such problems, I use Checkstyle. If you like I can post the configuration I use.

Sure, please do that.
How do you run checkstyle?

> Garbage collection: I like make garbage collection implementation independent; is it
OK for you if 
> I remove those cases?

Sorry, which cases?

> AbstractGarbageCollector and AbstractDataStore are not required then (anyway they are
small).

The AbstractDataStore class has members that are common to both data stores, and are implementation

independent. It can be removed, but we'll en up with a lot of duplicated code in both subclasses.
On the other hand, the AbstractGarbageCollector is easier, the only difference is that 
FileGarbageCollector overrides touchBinaryProperty(), which is different. I guess you can
merge both 
methods in one that calls back to the data store impl to update the record's time, but the
way I see 
it now is probably more complicated. Maybe you can tell me your idea on how to do this, so
I can 
understand better.

> It would be great if the database data store would automatically re-connect if the connection
was 
> lost (important for MySQL). To achieve this, 
> org.apache.jackrabbit.core.persistence.bundle.util.ConnectionRecoveryManager can be used.

This is our point of most concern.
The problem is that if we follow in the data store the pattern used in the PMs (only one 
connection), it won't be possible to have multiple clients concurrently uploading binaries.
That's 
why we did it that way.
Still, we don't think that adding reconnection is going to be a problem, but what concerns
us is 
that doing so forces the one connection pattern.
In our app we subclass the DatabaseDataStore class and override the getConection(boolean)
method to 
use a connection pool provided by the DataDirect JDBC drivers.

> It's quite tricky that DatabaseRecord extends FilterInputStream... However I'm not sure
if it is 
> required, is it not possible to just wrap the database BLOB object?

Yes, not only tricky but also very ugly. We had to it like that because the way our application

handles the binaries. But alas, we fixed that already. I'll send the changes as soon as I
figure out 
how to make a decent patch.

> I don't think that FileDataStoreConstants is required.

You're right, their just a leftover. Although I do think that the buffer size in FileDataStore

should be added as a constant, and maybe increased to 16 or 64KB

> Those are just my view, and I'm open to discuss them of course.
> If you have time to change it yourself please go ahead. Otherwise I will do it and post
the patch 
> here before I commit it - but it will take a few more days.

Do you have a patch against trunk?
If you do, please upload it, so I can update it with our latest changes and test cases.
After that I'll have no problem in doing these.
Looking at the ConnectionRecoveryManager, I think that its private methods should be delcared
as 
protected instead of private, because it should be possible to override them.

Regards,

Esteban Franqueiro
esteban.franqueiro@bea.com 


Notice:  This email message, together with any attachments, may contain information  of  BEA
Systems,  Inc.,  its subsidiaries  and  affiliated entities,  that may be confidential,  proprietary,
 copyrighted  and/or legally privileged, and is intended solely for the use of the individual
or entity named in this message. If you are not the intended recipient, and have received
this message in error, please immediately return this by email and then delete it.

Mime
View raw message