db-derby-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Van Couvering <da...@vancouvering.com>
Subject Re: [DERBY-2469] First alpha implementation available - help needed :)
Date Tue, 27 Mar 2007 22:19:05 GMT
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

Mime
View raw message