[RFC][Icedtea-web]: Changed how cache works.

[Andrew Su]

----- Original Message -----
> From: "Denis Lila" <dlila at redhat.com>
> To: "Andrew Su" <asu at redhat.com>
> Cc: "IcedTea" <distro-pkg-dev at openjdk.java.net>, "Omair Majid" <omajid at redhat.com>
> Sent: Thursday, April 14, 2011 1:54:01 PM
> Subject: Re: [RFC][Icedtea-web]: Changed how cache works.
> Hi.
> 
> You forgot to hg add CacheLRUWrapper. Assuming it was just a
> rename of the corresponding file in the previous version of
> your patch, I added it to your patch, and attached it. The
> attached patch should compile.

Oops, thanks for doing that!

> 
> Here are some things that jump out at me (more comments may come
> later on):
> 
> In getCacheFileIfExists, can you put the lastPath computation
> in it's own function (similar to getIdForCacheFolder). That way
> it'll be a bit easier to figure out what's going on.

Sure.

> 
> In removeUntrackedDirectories you do
> 
> + for (File f : temp.listFiles()) {
> + String path = f.getPath();
> + temp = new File(path);
> + if (temp.isDirectory() && !keep.contains(temp.getPath())) {
> + try {
> + FileUtils.recursiveDelete(temp, temp);
> 
> Wouldn't it be the same (and simpler) to do:
> 
> + for (File f : temp.listFiles()) {
> + if (f.isDirectory() && !keep.contains(f.getPath())) {
> + try {
> + FileUtils.recursiveDelete(f, f);
> 

Yes it would; Residue from moving things around..

> Also, the HashSet<String> argument of that function is called "keep".
> This is a bit peculiar, since you're deleting everything in it.
> Also, can you make that a Set<String> instead of HashSet?

Changed.

> 
> 
> Most importantly:
> > I have applied your patch and did some adjustments such as using the
> > hash of the url as file name instead of having a really long
> > directory.
> 
> Isn't this broken? I mean, what happens when two different urls
> hash to the same code?

I think it might be best to hold this off for now, and hack at it later.
This was an attempt to make it so that we don't have a bunch of folders
with just another folder in it.

> 
> To me, it seems like you're just going to create a file when it
> is first hashed, and when a different URL has the same hash
> getCachFileIfExist will return the first cached file, instead
> of creating a new one.



-- snip --
Cheers,
  Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 201104014_cache_stuff_vXX.patch
Type: text/x-patch
Size: 31998 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20110414/2c8b3eb7/201104014_cache_stuff_vXX.patch