db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luigi Lauro <luigi.la...@gmail.com>
Subject Re: [DERBY-2469] First alpha implementation available - help needed :)
Date Tue, 27 Mar 2007 20:23:04 GMT

On 27 Mar 2007, at 20:01, David Van Couvering wrote:

> Hi, Luigi.  Thanks for submitting this!

Thanks for replying.

> Here are my comments:
> - Your code is very well written - clear, well-commented, nicely  
> structured!

Thanks. I try to do my best, and I value code clarity above all.

My goal is that ppl should understand what I've written with the  
minimum effort possible.

> - Sigh.  Does the JNLP persistence service have no concept of a  
> directory?  I hate the "length == 0" approach of identifying a  
> directory.  But sometimes that's the best we can do.  Perhaps we  
> can provide some feedback to the JNLP folks?

Unluckily, no.

The structure is 'somewhat' hierarchical since it's like a Map<URL,  
FileContent>, but I'm forced to write under my codebase only. I  
cannot create 'virtual' directory in the URL.

This means that if I'm "http://db.apache.org/derby" (this is the  
codebase so), I can write under:


This is allowed in order to allow data sharing between applications  
from the same vendor/domain.

BUT, I cannot write to http://db.apache.org/derby/subdir. I get a  
'denied access' because PersistenceService enforces the codebase. I  
cannot go past it.

This is why I had to resort to using the NAME itself (which is  
completely free, as long as doesn't violate URLs standard) and code  
an IMPLICIT hierarchy into the name. Of the available separators  
(pipe, comma, etc...) I chose comma because it seems to me it's the  
least commonly used in 'files' and the most easily readable  
(dir,dir2,file is more readable than dir|dir2|file).

This means I will persist the file as


for example. For JNLP that's a single file under the http:// 
db.apache.org/derby codebase, so it's allowed, and I code the  
hierarchy into the name with comma separated paths.

Regarding directories: I've chosen to create directory even if they  
contain nothing (so I will create a placeholder entry whenever a  
directory is 'created', otherwise I would have no mean of find out if  
a directory exists or not, if no file is available under it) and I've  
chosen to use the 0-length to 'flag them'. Since they won't hold any  
data at all, this isn't _THAT_ unclean after all.

Other approaches I considered were to use the TAG feature of JNLP to  
tag directory, but I discarded it because I would have to abuse of  
the meaning of the TAG and violate the API contract, since the only  
supported values are DIRTY, CACHED, TEMPORARY. Or to flag them using  
the name, but this would add even more complications to the name  
structure, which already contains the path hierarchy and all, so I  
thought the 0-length was the best I could get.

> Don't you need to close any open files on shutdown?  This  
> highlights my ignorance of this area of the code...

They are really 'files', you should think of them more like cookies.

I don't need to close anything, I can create/delete any entity and  
write/read to/from it using streams.

Of course, the streams have to be flushed/close after usage, but  
that's not my duty: I make them available through the getInputStream/ 
getOutputStream methods on the StorageFile interface, then it's duty  
of the caller to close them upon usage.

Where does it seems to you I have left something open? Maybe I've  
done and haven't noticed, but AFAIK I can't see any such mistake.

Beware also that some stuff as sync/supportRws is still in the works  
for the JNLPStorageFactory. Not everything is in yet

> - Next time, please submit your changes as a patch file.  You  
> generate a patch file by doing 'svn diff' from the root of your  
> code tree, and routing the output to a file.  Make sure you have  
> actually added new files and directories in your local tree using  
> 'svn add'.

Will do, thanks for letting me now. I didn't make a patch of it  
mainly because these are new files, so I thought it was easier this  
way, but I will provide a patch next time :)

> - You didn't submit any unit tests.  Without this, how do we know  
> this feature works?  Have you run it through any tests?

Haven't put in any test yet. I don't think this code will run yet,  
very very few tests done on it, all manual. I plan to provide junit  
unit tests later, when I get the hang of it and stabilize the code a  
bit. Yes, I know TDD and all, but given the premises, I focused on  
understanding how derby works and testing my ideas of a possible  
implementation against reality :)

And also, I was hoping to find a full StorageFile/StorageFactory  
testing suite in derby for checking if the interface implementations  
fully adhere to the API specifications, but no such thing seems  
available to me. Are there some specific tests I could run against a  
StorageFactory/StorageFile implementation?

> - Did you run derbyall?  Did it pass?

Not at all. In fact, I have yet to see how I can 'pass' my new  
implementation to derby for testing. :) Any pointer here is appreciated

> Take a good gander at http://wiki.apache.org/db-derby/ 
> DerbyCommitHowTo - everything here applies except that you can't  
> commit the change yourself, only committers can do that.
> - Did you go through the checklist at http://wiki.apache.org/db- 
> derby/DerbyContributorChecklist?  This is really important,  
> especially signing and faxing in the ICLA

Will check these later. Thanks!

> - There are three versions of JNLPStorageFactory.java.  It's really  
> hard to tell which one is the most recent.  When submitting updated  
> patches, again it's best if you submit a single patch file, and  
> name it with a version number or a date.

True, in fact I was looking for some 'mod' to delete the old ones. I  
will submit a patch with date in the name, so to avoid doubts next  
time. Can you tell someone to clean those attachments in the  
meanwhile? :)

> - Your source files need to have the Apache license header on  
> them.  See other files for examples

I will, thanks again. I'm n00b here you know :)

> Gotta run, maybe more later.

Thanks again, looking forward for any more help.

Really really appreciated David.

View raw message