You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "shubhamvishu (via GitHub)" <gi...@apache.org> on 2023/11/11 19:05:25 UTC
[PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
shubhamvishu opened a new pull request, #12798:
URL: https://github.com/apache/lucene/pull/12798
### Description
This PR addresses [this comment](https://github.com/apache/lucene/blob/4e2ce76b3e131ba92b7327a52460e6c4d92c5e33/lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java#L113) in `HnswConcurrentMergeBuilder` to reuse the TaskExecutor and not duplicate the code.
It moves the logic of retrieving the results from futures in `TaskExecutor.TaskGroup#invokeAll` function to the newly added static function `getFutureResults` in `TaskExecutor`. The function takes a collection of `Future` and returns the list of retrieved results if required i.e. when the passed boolean `collect` is true else it just wait for the futures to complete and return null. I reused the added function in couple of places other than `HnswConcurrentMergeBuilder` in the codebase which seemed suitable.
<!--
If this is your first contribution to Lucene, please make sure you have reviewed the contribution guide.
https://github.com/apache/lucene/blob/main/CONTRIBUTING.md
-->
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on PR #12798:
URL: https://github.com/apache/lucene/pull/12798#issuecomment-1836093934
Exactly.... @javanna Do you think it would make sense to have a new `FutureUtil` class and add this function there?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12798:
URL: https://github.com/apache/lucene/pull/12798#discussion_r1390277613
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java:
##########
@@ -77,42 +75,21 @@ public OnHeapHnswGraph build(int maxOrd) throws IOException {
HNSW_COMPONENT,
"build graph from " + maxOrd + " vectors, with " + workers.length + " workers");
}
- List<Future<?>> futures = new ArrayList<>();
+ List<Future<Void>> futures = new ArrayList<>();
for (int i = 0; i < workers.length; i++) {
int finalI = i;
futures.add(
exec.submit(
() -> {
try {
workers[finalI].run(maxOrd);
+ return null;
} catch (IOException e) {
throw new RuntimeException(e);
}
}));
}
Review Comment:
Should we instead use the `TaskExecutor` here to run the tasks concurrently?. But since the `TaskExecutor` cx is currently pkg-private we would need to make it public to use here. It makes sense to me to do so since we are making use of concurrency/executors in indexing etc also(eg: hnsw) and not just at search time.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #12798:
URL: https://github.com/apache/lucene/pull/12798#issuecomment-1834417361
We do have some existing util classes but nothing that I recall around concurrency or futures handling.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #12798:
URL: https://github.com/apache/lucene/pull/12798#issuecomment-1839288615
I don't see a lot of value in sharing this code to be honest, as there are three places that simply call future.get and catch execution exception and unwrap the actual exception. That seems ok to duplicate to me.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on PR #12798:
URL: https://github.com/apache/lucene/pull/12798#issuecomment-1820748202
> Is this in fact superseded by https://github.com/apache/lucene/pull/12799 ?
Yes, there would be a conflict `HnswConcurrentMergeBuilder` class due to #12799 which we could resolve now(rest would be as is). I'll remove the changes of that class in this PR but otherwise rest of changes is ready for review.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on PR #12798:
URL: https://github.com/apache/lucene/pull/12798#issuecomment-1834080104
@javanna Hmm ....I agree with you the consumers don't really depend on TE to run their Futures and its more like a generic utility function being exposed by TE(which is better to avoid now that I'm thinking more and more). Maybe this sort of function would be more suited in some utility class under `org.apache.lucene.util` but I don't see any such obvious candidate. Let me know if you think there exists some other place for this function instead of TE, otherwise we could close this PR I think.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on code in PR #12798:
URL: https://github.com/apache/lucene/pull/12798#discussion_r1390277613
##########
lucene/core/src/java/org/apache/lucene/util/hnsw/HnswConcurrentMergeBuilder.java:
##########
@@ -77,42 +75,21 @@ public OnHeapHnswGraph build(int maxOrd) throws IOException {
HNSW_COMPONENT,
"build graph from " + maxOrd + " vectors, with " + workers.length + " workers");
}
- List<Future<?>> futures = new ArrayList<>();
+ List<Future<Void>> futures = new ArrayList<>();
for (int i = 0; i < workers.length; i++) {
int finalI = i;
futures.add(
exec.submit(
() -> {
try {
workers[finalI].run(maxOrd);
+ return null;
} catch (IOException e) {
throw new RuntimeException(e);
}
}));
}
Review Comment:
Should we instead use the `TaskExecutor` here to run the tasks concurrently?. But since the `TaskExecutor` cx is currently pkg-private we would need to make it public to use here.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #12798:
URL: https://github.com/apache/lucene/pull/12798#issuecomment-1820666255
Is this in fact superseded by #12799 ?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on PR #12798:
URL: https://github.com/apache/lucene/pull/12798#issuecomment-1839348896
Makes sense to me! I'll mark this as close then. Thanks @javanna !
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "javanna (via GitHub)" <gi...@apache.org>.
javanna commented on PR #12798:
URL: https://github.com/apache/lucene/pull/12798#issuecomment-1828210981
With the latest updates, I am not convinced about this change. I think it's great to use TaskExecutor to execute parallel tasks, like you did in #12799, but I am under the impression that this PR takes it a bit too far in terms of responsibilities of TaskExecutor and functionalities it exposes. I woudl be ok with those consumers that deal with futures keeping their current logic that does not depend on TaskExecutor, unless they use TaskExecutor directly to run their futures. What do you think?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Add static function in TaskExecutor to retrieve the results for a collection of Future [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu closed pull request #12798: Add static function in TaskExecutor to retrieve the results for a collection of Future
URL: https://github.com/apache/lucene/pull/12798
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org