Hi Jody, Thanks one more time for your detailed feedback and the proposal alternative, it is a good one. I made an implementation based on it and it works well, so I updated the GeoServer proposal as well and created the draft PRs for the community evaluation and review.
GeoTools PR: https://github.com/geotools/geotools/pull/3634 GeoServer PR: https://github.com/geoserver/geoserver/pull/5279 Regards, Fernando Mino == GeoServer Professional Services from the experts! Visit http://bit.ly/gs-services-us for more information. == Fernando Mino Software Engineer @fmy2kec GeoSolutions Group phone: +39 0584 962313 fax: +39 0584 1660272 https://www.geosolutionsgroup.com/ http://twitter.com/geosolutions_it ------------------------------------------------------- Con riferimento alla normativa sul trattamento dei dati personali (Reg. UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si precisa che ogni circostanza inerente alla presente email (il suo contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra operazione è illecita. Le sarei comunque grato se potesse darmene notizia. This email is intended only for the person or entity to which it is addressed and may contain information that is privileged, confidential or otherwise protected from disclosure. We remind that - as provided by European Regulation 2016/679 “GDPR” - copying, dissemination or use of this e-mail or the information herein by anyone other than the intended recipient is prohibited. If you have received this email by mistake, please notify us immediately by telephone or e-mail. On Wed, Sep 8, 2021 at 8:34 PM Jody Garnett <jody.garn...@gmail.com> wrote: > Fernando: > > Rather than watching communication go by: > - Moved your proposal to the description section (you had the proposal > outlined as tasks) and wrote in my assumption for MarkFactoryProcessor > interface. > - Wrote up alternate proposal more formally > > -- > Jody Garnett > > > On Thu, 12 Aug 2021 at 14:34, Fernando Mino < > fernando.m...@geosolutionsgroup.com> wrote: > >> Hi Jody, >> >> Thanks for the insight on Factory registry as a better alternative. >> Indeed the DynamicSymbolFactoryFinder.getMarkFactories() -> >> FactoryRegister.setOrdering(..) approach will do the trick for one of the >> scenarios we want to support (I will amend the proposal to add this to >> support the global configuration use case) but we also have the >> per-workspace use case that allows: >> - To have several different MarkFactory ordering and filtering on several >> workspaces. >> - To have several WMS GetMap requests (and rendering threads) in >> parallel, rendering Mark shapes with different MarkFactory implementations >> order and availability profiles. >> >> Probably I'm wrong (and please let me know if indeed there is a way to >> achieve this with DynamicSymbolFactoryFinder.getMarkFactories() -> >> FactoryRegister.setOrdering), but looks like the per-workspace >> configuration + multithreading support is not possible with FactoryRegister >> due to its not-thread-safe, globally cached and synchronized-static-methods >> caller(DynamicSymbolFactoryFinder). >> >> To use FactoryRegistry we would need to change DynamicSymbolFactoryFinder >> to allow us to store and index different FactoryRegistry instances per >> workspace, but the problem here is GeoTools Rendering module is not aware >> of GeoServer workspaces, and that is why I proposed the on-the-fly >> filtering and ordering interceptor extension point, to allow us to control >> this filtering from GeoServer code during request runtime. >> >> Naturally any better idea to solve this second use case is welcome. >> >> Regards, >> >> Fernando Mino >> >> == >> >> GeoServer Professional Services from the experts! >> >> Visit http://bit.ly/gs-services-us for more information. >> >> == >> >> Fernando Mino >> >> Software Engineer >> >> @fmy2kec >> >> GeoSolutions Group >> phone: +39 0584 962313 >> >> fax: +39 0584 1660272 >> >> https://www.geosolutionsgroup.com/ >> >> http://twitter.com/geosolutions_it >> >> ------------------------------------------------------- >> >> Con riferimento alla normativa sul trattamento dei dati personali (Reg. >> UE 2016/679 - Regolamento generale sulla protezione dei dati “GDPR”), si >> precisa che ogni circostanza inerente alla presente email (il suo >> contenuto, gli eventuali allegati, etc.) è un dato la cui conoscenza è >> riservata al/i solo/i destinatario/i indicati dallo scrivente. Se il >> messaggio Le è giunto per errore, è tenuta/o a cancellarlo, ogni altra >> operazione è illecita. Le sarei comunque grato se potesse darmene notizia. >> >> This email is intended only for the person or entity to which it is >> addressed and may contain information that is privileged, confidential or >> otherwise protected from disclosure. We remind that - as provided by >> European Regulation 2016/679 “GDPR” - copying, dissemination or use of this >> e-mail or the information herein by anyone other than the intended >> recipient is prohibited. If you have received this email by mistake, please >> notify us immediately by telephone or e-mail. >> >> >> On Wed, Aug 11, 2021 at 8:11 PM Jody Garnett <jody.garn...@gmail.com> >> wrote: >> >>> Thanks for doing this up as a proposal, it really helps with discussion. >>> >>> I did not think that MarkFactory or ExternalGraphicFactory needed to be >>> aware of priority as this is handled by FactoryRegistry; but you are >>> correct none of the existing implementations extend AbstractFactory. >>> >>> Your proposal is on MarkFactory; do you need a similar change for >>> ExternalGraphicFactory? >>> >>> I am not quite sure how MarkFactoriesProcessor is intended to work: >>> >>> - processFactories( Iterator<MarkFactory> factories ): >>> Iterator<MarkFactory> >>> Processing an iterator on the fly is good, it amounts to doing pair >>> wise ordering, or a comparator, which is already supported by >>> FactoryRegistry. >>> - priority(): int >>> What is the int for? Is it to order between competing instances >>> of MarkFactoriesProcessor ? >>> >>> >>> My hesitation is seeing more than one way to do things, especially if it >>> is a one-off for a specific factory. So I would like to see what the >>> limitation you are running into with the existing setup, and if your >>> proposal is an improvement we could do it for all FactoryFinders. We can >>> look at other factory finders for examples of helper methods used to >>> address common challenges (factory iterator providers for injecting >>> instances, establishing pairwise ordering >>> <https://github.com/geotools/geotools/blob/06d379a2fdbfdbf94641c16813c37ef02aa7f63c/modules/library/referencing/src/main/java/org/geotools/referencing/ReferencingFactoryFinder.java#L546>, >>> ...). I know that establishing pairwise ordering is a pain, and there is >>> already a helper method that takes a comparator >>> <https://github.com/geotools/geotools/blob/main/modules/library/metadata/src/main/java/org/geotools/util/factory/FactoryRegistry.java#L1265> >>> to make this easier. >>> >>> Checking your proposal you reference propose changing the code here >>> <https://github.com/geotools/geotools/blob/2c0f22f0a5a13885aa1d85faa6f715ea87f3c90e/modules/library/render/src/main/java/org/geotools/renderer/style/SLDStyleFactory.java#L1362> >>> : >>> >>> *Iterator<MarkFactory> it = >>> DynamicSymbolFactoryFinder.getMarkFactories();* >>> >>> >>> I would ask instead that you change DynamicSymbolFactoryFinder directly; >>> that interface is already responsible for SPI discovery, >>> >>> Here is a proposed alternative approach: >>> >>> 1. Leave StyleFactory alone (it already has too much logic) and >>> trust DynamicSymbolFactoryFinder.getMarkFactories() to respect your >>> ordering, especially since you are trying to work around how factory >>> discovery is based on classpath order. >>> >>> 2. Create a method DynamicSymbolFactoryFinder setMarkFactoryOrder( >>> Comparator<MarkFactory> comparator ): >>> >>> public static synchronized >>> Iterator<MarkFactory> setMarkFactoryOrder( Comparator<MarkFactory> >>> comparator ) { >>> getServiceRegistry().setOrdering( MarkFactory.class, comparator >>> ); >>> } >>> >>> 4. This calls FactoryRegister.setOrdering( category, comparator ) >>> <https://github.com/geotools/geotools/blob/main/modules/library/metadata/src/main/java/org/geotools/util/factory/FactoryRegistry.java#L1265> >>> which >>> should do the trick. >>> >>> >>> -- >>> Jody Garnett >>> >>
_______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
