Hi Jody,
Thanks for your review.
On 17-12-15 00:36, Jody Garnett wrote:
> - architecture reason: I was trying to have ResourceStore be the
> single source of truth for resources (including change notification).
> Is there a reason why this should change?
Yes and no.
There is a very clear requirement behind this: Resource notifications
need to be sent in a clustered environment. (the main purpose of the
jdbcstore at this moment is clustered environments. )
So we are left with ONLY these three possible options:
(1) Enforce every one who uses the jdbcstore to use hazelcast (BAD)
(2) Enforce every one who uses hazelcast to use the jdbcstore (BAD)
(3) Add an additional abstraction for the distribution of resource
notifications, kept separate from the ResourceStore (which I called
ResourceWatcher).
There is no alternative, if you agree that (1) and (2) are bad options,
you simply must agree with point (3).
BUT, the default store, the FileSystemBasedStore is not intended to use
with clustering (that is why we made a file independent store in the
first place). As such, the FileSystemBasedStore has no use for this
additional abstraction. But that is no problem: because this isn't being
enforced. The ResourceStore already provides the API needed for adding
handlers to resources and this remains unaltered, the programmer who
uses Resources need not worry about the existence of the ResourceWatcher
as they never need to use it. Alternative implementations of
ResourceStore may choose to use it, which would be recommended if they
wish to support clustering, but that is all.
>
> Two questions:
> - If you are really focused on event dispatch can we rename the
> interface to match?
Sure, ResourceNotificationDispatcher then.
> - Should we have an event dispatch mechanism that works in a similar
> fashion for catalog and resource change events?
Bit confused, resource change events is what we are talking about.
Catalog: good point. I don't know if there is a mechanism in place for
distributing these in a clustered environment, but if not we do need
such a mechanism. It is possible this can be solved with a different
mechanism.
>
> 2) New utility methods
>
> Resources.toURL( resource )
> - makes a lot of sense and reads better in the code
>
> I am not too worried about additional utility methods, instead I am
> concerned about changes to objects we may have to implement (such as
> GeoServerResourceLoader).
>
> GeoServerResourceLoader.fromURL(String url)
> GeoServerResourceLoader.fromURL(URL url)
> - yeah I know you did not write this - just added one that takes a URL
> (do you really need both?)
Yes, you do. The fromURL(String) method accepts things that would be
rejected by a normal URL parser, abolishing it would be a serious
regression causing many configurations to fail.
>
> GeoServerResourceLoader.fromPath(String path) {
> - I like that it is consistent with the from URL methods...
> - These are a bit awkward as they add API to GeoServerResourceLoader
> (trying to encourage code to just use ResourceStore and utility classes)
> - Actually fromPath just delegate to Resources utility class ...
>
> public Resource fromPath(String path) {
> return Resources.fromPath(path, resources.get(Paths.BASE));
> }
>
> So no need to add helper method here use:
> Resource r = Resources.fromPath( path, loader.get(Paths.BASE));
>
> Only thing I can think of is that you are trying to hide the fact we
> have a DATA_DIRECTORY used as starting point?
These are pure convenience methods. I made them in analogy to the
previously already existing GeoServerResourceLoader.url convenience
method, which works on the exact same principle and was already there.
If you insist they can be removed, but I personally don't see the harm
and I find them neater to use than having to call Paths.BASE etc... But
it's up to you, say the word.
Regards
Niels
------------------------------------------------------------------------------
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel
