[GitHub] [jena] afs commented on pull request #1063: JENA-2154: Custom SERVICE executors

[GitBox <gi...@apache.org>]

afs commented on pull request #1063:
URL: https://github.com/apache/jena/pull/1063#issuecomment-912978339


   This is looking like good timing. While I, personally, think using SERVICE for extensions for stored procedures isn't perfect, it does stay within standard syntax which is valued by users and has some take-up.
   
   Thanks for following existing design patterns (even if they are, with hindsight a bit overly complex in places!).
   
   Starting with design review:
   
   _Deferred start_
   
   Delayed initialization can handled orthogonally. See
   `IteratorDelayedInitialization` for example. `Supplier` is coping with an uncommon case
   that I think can be done within an implementation anyway.
   
   _Location_
   
   > Probably it is indeed better to move the code from QueryIterService to Service - but add the 'Binding' argument to the function in Service
   
   Possible, though I don't think it make much difference either way.  Leave it in `QueryIterService` for now?
   
   _Default_
   
   I'd like to handle the default case (in the PR, no factory found) as a regular `ServiceExecutorFactory` added from `ARQ.init` rather than code here. It is nice to have a "really no SERVICE support" done by clearing the registry even if we keep the old context approach.
   
   The code will need to handle "no factory found" as `QueryExecException` or a join-identity
   query iterator depending on `SILENT`.
   
   _Substituted vs original op_
   
   This isn't so easy to treat as separate.
   
   * A dispatch may be `SERVICE ?service` with `?service` already bound but the
   registration is to `<someURI>`.
   
   * This code is in `nextStage` so iteration is happening further out. If the implementation wants bottom-up only, might have to have a different optimizer - i.e. needs to take a wider view. It can still do a single execution and keep that internally (`QueryIterator.close` will be called for clear-up).
   
   I don't think that is common so the API can focus on the usual case because the other can still be done as long as the original is available.
   
   How about providing both in the factory call so the implementation is informed?
   
   With those points:
   
   ```java
   public interface ServiceExecutorFactory {
       QueryIterator createExecutor(OpService op, OpService original, Binding binding, ExecutionContext context);
   }
   ```
   
   The switch to `ExecutionContext` is a good idea.
   
   _Timing_
   
   We are close to the 4.2.0 release point. As an extension to advertise, we should take the time to get it right. A whole cycle (3-4 months) is a bit long so my suggestion is include in 4.2.0 with the understanding that it is "Experimental - work-in-progress - subject to refinement".
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org