Comments inline.
On 24 July 2015 at 07:30, Niels Charlier <ni...@scitus.be> wrote:
> On 23-07-15 18:45, Jody Garnett wrote:
>>
>> I implemented replacement watchers as part of the migration? Are they not
>> working for you ...
>> And that code should still work, the directory will get unpacked - but it
>> is not any kind of fun.
>>
>
> If the changes are picked up and written to the db properly..
>
I think it was a one way migration disk to JDBCConfig. I was thinking
specifically of the FileWatcher class which was changed to accept a
Resource. It should be able to listen to a resource changing and notify
code if required. The file based implantation checks file timestamps in the
background, although Java 7 has event notification for this.
Not sure how you want to handle this for JDBCConfig?
Still we may be thinking in a different direction. The initial import from
data directory to JDBCConfig is a one time journey. After that the idea is
to manage via REST API (yes ResourceStore still needs a REST API).
It sounds like you are considering editing files on disk, and then
reimporting them as they change? That is a very tricky problem since the
data directory would then be a local cache *and* and place for editing.
> I am having some issues with watching actually. Can we talk about this on
> skype or IRC ? Are you online today?
>
I am online for a bit, will join IRC now and see if you are there.
>
> Either way, you have to admit it is not the ideal way of doing this,
> writing data to the file system and secretly picking this up in another
> thread and writing it to the database. It's also very hard to test, at this
> moment the resourcestore test assume that changes happen immediately. I
> found that I had to put in a delay of 20 seconds (!) for the test to work
> on the jdbcresourcestore, but there is no guarantee that this will work
> always and everywhere within that time frame. We cannot possibly know when
> the other thread has finished updating the database, can we? How would you
> suggest testing it?
See above, import should be a one time trip. Edit via REST API where you
can tell when change is written to database (and issue change notification
for other nodes in the cluster).
You statement is a bit circular. Yes you can use dir() to create the
>> folder on the file system if needed (just like you can use file() to create
>> a file). Is there anything else you need that is not provided?
>>
>> Like who cares if the directory is created or not, as long as it behaves
>> the same... It is up to your store implementation if you want an internal
>> representation of a directory or not. I think you can create it in a lazy
>> fashion as needed while respecting the api ...
>>
>> Is directory creation or not just an internal detail?
>>
>
> Do we agree on this: the file() and dir() methods are only for backwards
> compatibility and should be discouraged (deprecated in my opinion).
I don't completely agree, we have a few sections of the code that require
file(). See the initial design doc for details (it was one of templating
things (GeoServerTeamplateLoader?) and perhaps Fonts in the styles
directory.
Since dir() is a short cut for going through list() and calling file() it
would remain undeprecated.
People should be use in() and out() instead, right?
Yes, we should only have a couple uses of file() in our application at the
end of the day - and they will be very specific.
> If you want to really use the resource system, and NOT the file system,
> there is no way of making sure a directory exists in the resource store
> (DB).
If you feel this is necessary, go ahead, I just do not see the need myself.
To list contents, rename, etc...
>>
>
> Indeed, but we can only do all of these things if the directory already
> exists. And we cannot easily force it to exist if we we need it to exist
> (see my example code). Basically, it is not possible to create an empty
> directory unless you create a dummy file and then delete it.
I saw your example code, I just do not understand why your code needs the
directory to exist. That is why I was hoping for a real example from the
codebase...
Okay I see your concern. If you list() an undefined resource you get an
>> empty list .. can you use a UNDEFINED resource and trust it to behave as a
>> directory for your code?
>>
>
> I thought list() would throw an exception, but it actually returns null. I
> guess you have a point there, just leaving it as undefined would work. It
> does require us to do a null check, but okay. Some operation like rename
> might also require us to check if it actually exists, while if we knew for
> sure it did we didn't have to worry. It seems like it would add more ifs to
> the code, often needing to verify whether it is actually a directory yet or
> not.
>
I would prefer to set rename up to be cheerful to rename undefined things,
idea is to remove null checks and special cases where possible. So if you
need an undefined resource to return an empty list you could make that
change ... indeed that may be smart so code like the following can work
with out modification:
for (Resource r : resource.list() ){
...
}
Also , if the existence of directories is to an implementation detail
> hidden from the users, why is it possible to delete (but not to create an
> empty) directory?
>
> This would help if you can hunt down an example from the codebase :)
>> Especially if you want to replace FileWatch ...
>>
> Anything that calls findOrCreateDirectory().
>
You know resource.dir() acts like find or create a directory right? It will
make the directory if needed ...
Okay here is the search:
https://github.com/geoserver/geoserver/search?utf8=✓&q=findOrCreateDirectory
src/community/python/src/main/java/org/geoserver/python/Python.java
<https://github.com/geoserver/geoserver/blob/d54d57219141b9d8806307c1f02228c0008912c4/src/community/python/src/main/java/org/geoserver/python/Python.java>
- This is a community module so it is not really our concern to port it.
- That said it looks like it could be ported to resource use if required,
if the script languages really want a directory to play with than using
dir() to unpack a directory with all the scripts should be fine.
src/extension/csw/core/src/main/java/org/geoserver/csw/store/internal/GeoServerInternalCatalogStore.java
<https://github.com/geoserver/geoserver/blob/d54d57219141b9d8806307c1f02228c0008912c4/src/extension/csw/core/src/main/java/org/geoserver/csw/store/internal/GeoServerInternalCatalogStore.java>
- replace with Resource use
src/community/jdbcconfig/src/main/java/org/geoserver/jdbcconfig/internal/JDBCConfigPropertiesFactoryBean.java
<https://github.com/geoserver/geoserver/blob/d54d57219141b9d8806307c1f02228c0008912c4/src/community/jdbcconfig/src/main/java/org/geoserver/jdbcconfig/internal/JDBCConfigPropertiesFactoryBean.java>
- not sure about this, creating a base directory is fine, but that would
also be created as needed
- this class also has a method to create the scripts directory - although
see above scripts is a community module
src/community/w3ds/src/main/java/org/geoserver/w3ds/utilities/X3DInfoExtract.java
<https://github.com/geoserver/geoserver/blob/d54d57219141b9d8806307c1f02228c0008912c4/src/community/w3ds/src/main/java/org/geoserver/w3ds/utilities/X3DInfoExtract.java>
- should just be using dir()
src/extension/importer/rest/src/main/java/org/geoserver/importer/rest/TaskResource.java
<https://github.com/geoserver/geoserver/blob/d54d57219141b9d8806307c1f02228c0008912c4/src/extension/importer/rest/src/main/java/org/geoserver/importer/rest/TaskResource.java>
- example of file system based code, not using resource store
src/community/wms-eo/src/main/java/org/geoserver/wms/eo/EoStyleCatalogListener.java
<https://github.com/geoserver/geoserver/blob/d54d57219141b9d8806307c1f02228c0008912c4/src/community/wms-eo/src/main/java/org/geoserver/wms/eo/EoStyleCatalogListener.java>
- setting up for copyUrlToFile - should be able to use one of the resource
utility classes for this
src/restconfig/src/main/java/org/geoserver/catalog/rest/FreemarkerTemplateResource.java
<https://github.com/geoserver/geoserver/blob/d54d57219141b9d8806307c1f02228c0008912c4/src/restconfig/src/main/java/org/geoserver/catalog/rest/FreemarkerTemplateResource.java>
- Some file upload code, should be able to convert to Resource use
src/community/w3ds/src/main/java/org/geoserver/w3ds/web/X3DLayerConfigPanel.java
<https://github.com/geoserver/geoserver/blob/d54d57219141b9d8806307c1f02228c0008912c4/src/community/w3ds/src/main/java/org/geoserver/w3ds/web/X3DLayerConfigPanel.java>
- Convert to resource use, a TileSetParser wants to be pointed at a
directory
src/gwc/src/main/java/org/geoserver/gwc/layer/DefaultTileLayerCatalog.java
<https://github.com/geoserver/geoserver/blob/d54d57219141b9d8806307c1f02228c0008912c4/src/gwc/src/main/java/org/geoserver/gwc/layer/DefaultTileLayerCatalog.java>
- trying to get access to the base directory for tile sets
So when you go through these consider what the code is about:
- if it is getting access to configuration it should use the resource store
API (and thus JDBCConfigResourceStore) in(), out(), etc...
- if it is needing a configuration file on disk (for use with a third-party
library) it should use resource.file() or resource.dir() as appropriate
(for use of Fonts, Templates).
- If it is needing a cache (for example GWC tile cache) then it should be
using Java File API (since such things are defined by environmental
variables and may (or may not) be in the data directory they will not end
up using the Resource Store API
Not sure what you mean with replacing FileWatch?
https://github.com/geoserver/geoserver/blob/master/src/main/src/main/java/org/geoserver/security/file/FileWatcher.java
Have a look at the above, it is used by security system to "watch" for
configuration changes.
Jody
------------------------------------------------------------------------------
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
