Aside; do we need to make the default value for this hint alphabetical? To match prior tomcat behavior letting folks upgrade without configuration change. -- Jody Garnett
On Wed, 8 Sept 2021 at 07:07, Jody Garnett <jody.garn...@gmail.com> wrote: > Thanks Fernando I appreciate you taking the time to revise the proposal. > +1 > > -- > Jody Garnett > > > On Mon, 6 Sept 2021 at 08:09, Fernando Mino < > fernando.m...@geosolutionsgroup.com> wrote: > >> Thanks Jody for your hints. >> >> Indeed, using rendering hints to pass the pre-process behavior to >> GeoTools makes sense, it helps us to avoid adding a new extension point and >> I added this approach to the proposal. Since we need different behavior >> per-workspace we'll make rendering evaluate the current feature, and make >> GeoServer cache the per-feature-namespace result during the current >> rendering task in progress for performance optimization. >> >> The proposals were updated for your revision and feedback: >> >> https://github.com/geotools/geotools/wiki/Rendering-pre-process-Mark-Factories-Hint >> https://github.com/geoserver/geoserver/wiki/GSIP-204 >> >> >> 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 Fri, Aug 13, 2021 at 12:30 AM Jody Garnett <jody.garn...@gmail.com> >> wrote: >> >>> Right, sorry I missed that requirement earlier. >>> >>> For the workspace, or GetMap, use case that is a clear need for using a >>> Hint (pass your comparator in as a hint and make a getMarkFactories( hints >>> ) method; it probably already should of had one. >>> >>> Jody >>> >>> On Thu, Aug 12, 2021 at 2:34 PM 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 >>>>> >>>> -- >>> -- >>> Jody Garnett >>> >>
_______________________________________________ GeoTools-Devel mailing list GeoTools-Devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/geotools-devel
