[rfc][icedtea-web] Allow cache directory to be changed during runtime

[Jie Kang]

----- Original Message -----
> On 01/23/2015 08:09 PM, Jie Kang wrote:
> >
> >
> > ----- Original Message -----
> >> On 01/15/2015 07:16 PM, Andrew Azores wrote:
> >>> On 01/05/2015 02:49 PM, Jie Kang wrote:
> >>>> Hello,
> >>>>
> >>>> This patch consolidates the cacheDir instance to a single place and
> >>>> allows
> >>>> us to change it during
> >>>> run-time. It also modifies a unit test to use this new feature. This
> >>>> feature is aimed mainly at
> >>>> unit tests that need the cache, but do not want to modify the
> >>>> icedtea-web's user cache.
> >>>>
> >>>> Thoughts? Okay to push?
> >>
> >> I'm wondering why it is needed at all.
> >>
> >> JNLPRuntime.getConfiguration().getProperty(DeploymentConfiguration.KEY_USER_CACHE_DIR);
> >> is the place
> >> where you can set the dir.
> >>
> >>
> >> For unittest you may force refelction to do the job. if the chnage of the
> >> property isnot enough.
> >>
> >>
> >> What I would rather see is, to get rid of all the static stuff in
> >> CacheLRUWrapper and have
> >> possibility to create in instance.
> >> This instance will be also much more testable. But maybe it is overkill.
> >
> > Hello,
> >
> > Yes this is mainly to address that for unit tests, JNLPRuntime.[...]
> > doesn't work since CacheLRUWrapper and CacheUtil both use static variables
> > that get initialized on first use of either class. I think getting rid of
> > all the static stuff in LRUWrapper and making it instantiable is a great
> > idea. I see this patch as a baby-step towards doing that.
> >
> > I very strongly prefer NOT using reflection to set the cacheDir value in
> > the unit tests.
> >
> > Also, please see [1] for a longer explanation
> >
> > [1]
> > http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2015-January/030470.html
> >
> >
>   Yes. But still - if do this, do it properly, and so go with :
> 
> " to get rid of all the static stuff in
>   CacheLRUWrapper and have
>   possibility to create in instance.
>   This instance will be also much more testable"
> 
> and so go for singleton instead of static.

Hello,

Well... I took a look at the code

CacheLRUWrapper is already a singleton. After my patch, it contains a single static method: getInstance() which returns the instance. There is a single constant for the name of the recently-used file which is a private static final String. I think this is acceptable.

If you see the original patch, I actually already removed the static things in CacheLRUWrapper lol... my bad;


Regards,

> 
> 
> J.
> 
> 
> >
> >
> >>
> >> J.
> >>>>
> >>>>
> >>>> Regards,
> >>>>
> >>>
> >>> I think this looks generally ok. Maybe as a separate enhancement
> >>> changeset,
> >>> but since you're working
> >>> in here anyway, could you look at doing some refactoring to store the
> >>> cache
> >>> directory as something
> >>> better than a String? As a File, or using Java 7's nio2 stuff, a Path
> >>> maybe?
> >>
> >> On one side - yes, on the other - why? What operations are mainly done on
> >> the
> >> "string" string ops or
> >> file ops - I have not checked code, but seems like concate is most often
> >> :)
> >>
> >> But yes - it is directory, so file or path is probably better then string.
> >> Please, do this refactoring as separate changset anyway.
> >>
> >> J.
> >>
> >
> 
> 

-- 

Jie Kang

OpenJDK Team - Software Engineering Intern