Hi Jody, thanks for your feedback.
The problem with the alphabetical default on class names would be we will have SVG mark factory evaluated before WKT mark factories, which is the current performance killer scenario. Our backward compatibility strategy is to return the mark factory iterator as the original without any filtering or ordering, allowing any mark factory extension to be present and evaluated by the rendering code.
To match our previous Tomcat version classloader behavior mark factories order, which is alphabetical order by JAR module name class loading, we need:
-
Mark the mark factories to know the jar module name for every one, and establish the default order using the jar name. Or,
-
Add a priority field on the MarkFactory interface and implement the appropriate priority on core mark factories to make the default iterator follow the old tomcat classloader behavior. Or,
-
Hard code the original previous Tomcat versions factories order as default iterator result (maybe the worst idea).
Probably this is out of the scope of this proposal, since it is only to allow a custom filter and order by workspace configuration for Mark Factories on GeoServer WMS GetMap, but I agree it would be great to reach the original performance without the need of configuring it, I’m thinking maybe is material for a new proposal only on GeoTools side to make this default happen on the default factories code and define a good plan to accomplish it.
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 9:17 AM Jody Garnett <jody.garnett@anonymised.com> wrote:
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.garnett@anonymised.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.mino@anonymised.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.garnett@anonymised.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.mino@anonymised.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.garnett@anonymised.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 factories ): Iterator
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, …). I know that establishing pairwise ordering is a pain, and there is already a helper method that takes a comparator to make this easier.
Checking your proposal you reference propose changing the code here:
Iterator 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:
-
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.
-
Create a method DynamicSymbolFactoryFinder setMarkFactoryOrder( Comparator comparator ):
public static synchronized Iterator setMarkFactoryOrder( Comparator comparator ) {
getServiceRegistry().setOrdering( MarkFactory.class, comparator );
}
- This calls FactoryRegister.setOrdering( category, comparator ) which should do the trick.
–
Jody Garnett
–
–
Jody Garnett
