Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved custom service executor extension system #1387

Closed
Aklakan opened this issue Jun 15, 2022 · 4 comments · Fixed by #1388
Closed

Improved custom service executor extension system #1387

Aklakan opened this issue Jun 15, 2022 · 4 comments · Fixed by #1388
Labels
enhancement Incrementally add new feature

Comments

@Aklakan
Copy link
Contributor

Aklakan commented Jun 15, 2022

Version

4.6.0-SNAPSHOT

Suggestion

This is the issue for factoring out the extension system itself from #1314 so it can be reviewed in isolation.

Link to PR: #1388 (see it for detailed comments)

Are you interested in contributing a solution yourself?

Yes

@Aklakan Aklakan added the enhancement Incrementally add new feature label Jun 15, 2022
Aklakan added a commit to Aklakan/jena that referenced this issue Jun 15, 2022
Aklakan added a commit to Aklakan/jena that referenced this issue Jun 15, 2022
Aklakan added a commit to Aklakan/jena that referenced this issue Jun 15, 2022
@afs
Copy link
Member

afs commented Jun 17, 2022

Please explain what is going on here and why these changes are necessary

This is a repeat comment from before - I am concerned that changes to central machinery for one particular usage will have consequences for other users that are hard to predict.

Did you consider batching inside the service execution reading from the nextStage(Binding)?

Why is single treated different from a bulk of one?

@Aklakan
Copy link
Contributor Author

Aklakan commented Jun 17, 2022

Did you consider batching inside the service execution reading from the nextStage(Binding)?

Batching based on "push-events" to nextStage(Binding) is extremely complex to implement when the rest of ARQ is "pull" based - i.e. operators are implemented as iterators that pull as many items as needed from their underlying inputs in order to produce the next item.

As an example why I am not blindly batching by n bindings:

It would be easily possible adding a subclass of QueryIterRepeatApplyBulk that calls an abstract nextStage(Binding[] batch) method.

However, e.g. in #1314 I created conceptually a IterBatchByService (the actual naming is currently a bit awkward) which is an iterator over batches targeted at the same service and which is sourced by the query iterator of the lhs. It aims to create batches of a fixed size but it also adheres to a read ahead limit as to avoid consuming all of the lhs while attempting to fill a batch; or adding items to a batch that are too far ahead.

This is a bit smarter than blindly batching unrelated bindings.
Doing this with a push-based API (that expects immediate return values) would be hell.

Why is single treated different from a bulk of one?

Well, the single API is a special case of the bulk one - and the bulk one delegates to it. So its not really treating it differently but rather having two reasonable levels of abstraction (That's why I let QueryIterRepeatApply extend from QueryIterRepeatApplyBulk) - maybe the latter should be called simply QueryIterFlatMap.
Single is certainly more easy to implement - and so far this was how SERVICE was implemented in the first place!
Bulk is more powerful because it hands out the QueryIterator but that may require more effort (and knowledge) to work with.

@Aklakan
Copy link
Contributor Author

Aklakan commented Jun 18, 2022

I gave the QueryIteratorRepeatApply more thought and it turns out the extension system doesn't need a dedicated QueryIterator at all. Only the OpExecutor has to delegate the OpService case to the extension registry rather than directly creating a QueryIterService instance. Handlers in the registry for the bulk case (which receive the input QueryIterator) can choose to wrap that iterator with their own one (may be repeat-apply or some other) or pass them on to the next registered executor.

This means I can revert QueryIterRepeatApply.

I am also looking into combining the bulk and single registry into a one class (I am just not yet sure whether this internally uses the two existing registries or its own set of attributes).

@Aklakan
Copy link
Contributor Author

Aklakan commented Jun 18, 2022

W.r.t. to the comment that this is just for my solution: The bigger picture is that I would really love to have an infrastructure/path where it is possible to combine SERVICE plugins of different third-party systems such as those of sparql-anything (this is their OpExecutor*) - and of course those developed in my group. (Whether and when they'd upgrade is of course their choice.)

Back then I reached out with sparql-anything issue 60
Whenever you think this issue is in a state where its beneficial to reach out to them for comments I can do so.
(IMO I should first make a stable proposal for the revised registry though)

* Most work there happens in OpService; yet there is one override for OpBGP which extracts options from triples - similar to what stardog does. This would also be interesting to have as a future extension point but I don't think this has to be be a concern for this PR.

Aklakan added a commit to Aklakan/jena that referenced this issue Jun 18, 2022
Aklakan added a commit to Aklakan/jena that referenced this issue Jun 18, 2022
Aklakan added a commit to Aklakan/jena that referenced this issue Jun 23, 2022
Aklakan added a commit to Aklakan/jena that referenced this issue Jun 28, 2022
Aklakan added a commit to Aklakan/jena that referenced this issue Jun 28, 2022
Aklakan added a commit to Aklakan/jena that referenced this issue Jun 30, 2022
Aklakan added a commit to Aklakan/jena that referenced this issue Jun 30, 2022
@afs afs closed this as completed in #1388 Jul 3, 2022
afs added a commit that referenced this issue Jul 3, 2022
GH-1387 Improved custom service executor extension system
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Incrementally add new feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants