Hi, Luigi. Thanks for submitting this!
Here are my comments:
- Your code is very well written - clear, well-commented, nicely
structured!
- 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?
- Don't you need to close any open files on shutdown? This highlights
my ignorance of this area of the code...
- 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'.
- You didn't submit any unit tests. Without this, how do we know this
feature works? Have you run it through any tests?
- Did you run derbyall? Did it pass?
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
- 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.
- Your source files need to have the Apache license header on them. See
other files for examples
Gotta run, maybe more later.
Thanks,
David
Luigi Lauro wrote:
Compiles but doesn't run yet, a lot of things to check/do yet, but a
first 90% complete implementation is available.
You can download the latest versions for the 3 classes here:
https://issues.apache.org/jira/browse/DERBY-2469
I'm coding around a VirtualStorageFactory abstract base class, and a
VirtualStorageFile. VirtualStorageFile implements basic logic and
delegates to VirtualStorageFactory for the actual storage manipulation.
VirtualStorageFactory base class is a reworked from scratch
'BaseStorageFactory' which instead of forcing implementors to give both
StorageFile and StorageFactory implementation, allows them only to
implement a few 'CRUD' create/delete/rename/delete methods, and build
upon these to give a working StorageFactory/StorageFile implementation.
Ideally, if things go as I plan, every storage implementation could be
re-written around this new base class if needed, or this could be used
as a new base for developing new implementations in a easier way.
Have a look at the abstract class (VirtualStorageFactory) and the
delegating implentation of StorageFile (VirtualStorageFile).
I also include a starting JNLP implementation of VirtualStorageFactory.
Javadoc comments comes mostly from interfaces (copy & paste), and VERY
FEW TESTS DONE YET, this is very alpha code, if you wanna help go
through it and suggest fixes/improvements/whatever to get things rolling
for real.
I've probably done 100000000000000000000000 things wrong, so if someone
can help me spot what is right what is not, I can start to fix stuff and
provide a beta version soon. Thanks :)
P.S. Classes names are ugly, I plan to change those soon.
P.P.S. As soon as I get a beta JNLP implementation out, I will try to
provide a in-memory implementation as well. This is very easy to do,
given the base class.
