Hi, Luigi, thanks for your responses.
I think everything is covered, except for the question of testing. I
recognize you just wanted to get something started and some feedback.
But before anything gets checked in, I would need to see some validation
that it works, even partially works.
Thinking about it further, rather than building new tests, I guess all
you need to exercise this code is to modify the modules.properties file
to use this storage factory, and then run some subset of the Derby unit
tests.
I don't know the storage area very well, but I did find the following:
java/testing/org/apache/derbyTesting/unitTests/store
I have never run these tests, but if you ask on the list with a new
subject header you should get some help. Also, any help you may need on
configuring Derby to use your storage factory. What is unclear to me is
how to automate running in a Java Web Start environment, vs. in a
standard no-sandbox VM. Do you know how to do this?
I also want to see at least some subset of our higher-level system tests
run under the JNLP storage factory configuration. Most of the times the
database created is quite small, so I don't think it should be an issue.
In particular, I'm be curious to see what security bugs/issues we hit
as Derby tries to do various things with the system under such a
constrained SecurityManager as JWS.
In terms of running derbyall, a general principle is that no checkin
takes place without running derbyall. However, I do recognize that in
your case this is not necessarily applicaable since your code will never
be hit by running the existing regression suites.
What I would like to see is that one you have identified new tests or
new configurations of old tests that exercise your storage service, then
these new tests/configurations should be added to derbyall. I don't
think this is required for initial checkin, though.
Make sense?
Thanks,
David
Luigi Lauro wrote:
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:
http://db.apache.org/derby
http://db.apache.org
http://apache.org
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
http://db.apache.org/derby/derbyhome,databasehome,directory,subdirectory,filename
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.
Luigi
