You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "zacharymorn (via GitHub)" <gi...@apache.org> on 2024/03/28 22:30:55 UTC
[PR] GITHUB-13218: Add migrate entry for Collector to CollectorManager migration [lucene]
zacharymorn opened a new pull request, #13238:
URL: https://github.com/apache/lucene/pull/13238
This is a PR to add migration entry for `Collector` to `CollectorManager` migration for `IndexSearch#search`.
Thanks @mikemccand for the suggestions on entry content!
Note: I have some local gradle jvm issue that's blocking me to run a complete gradle check, but would like to share this in advance while I'm fixing the issue.
--
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] GITHUB-13218: Add migrate entry for Collector to CollectorManager migration [lucene]
Posted by "zacharymorn (via GitHub)" <gi...@apache.org>.
zacharymorn commented on PR #13238:
URL: https://github.com/apache/lucene/pull/13238#issuecomment-2038118749
After merge, I saw a test run error and this issue is fixing it https://github.com/apache/lucene/issues/13272
--
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] GITHUB-13218: Add migrate entry for Collector to CollectorManager migration [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on code in PR #13238:
URL: https://github.com/apache/lucene/pull/13238#discussion_r1544270671
##########
lucene/MIGRATE.md:
##########
@@ -185,6 +185,34 @@ enum.
`IOContext#LOAD` has been replaced with `IOContext#PRELOAD`.
+### IndexSearch#search(Query, Collector) being deprecated in favor of IndexSearcher#search(Query, CollectorManager) (LUCENE-10002)
+
+`IndexSearch#search(Query, Collector)` is now being deprecated in favor of `IndexSearcher#search(Query, CollectorManager)`,
+as `CollectorManager` implementation would allow taking advantage of intra-query concurrency via its map-reduce API design.
+To migrate, use a provided `CollectorManager` implementation that suits your use cases, or change your `Collector` implementation
+to follow the new API pattern. The straight forward approach would be to wrap the single-threaded `Collector` into a degenerate `CollectorManager`.
Review Comment:
Not a native speaker, but the use of `degenerate` looks surprising to me in that context?
##########
lucene/MIGRATE.md:
##########
@@ -185,6 +185,34 @@ enum.
`IOContext#LOAD` has been replaced with `IOContext#PRELOAD`.
+### IndexSearch#search(Query, Collector) being deprecated in favor of IndexSearcher#search(Query, CollectorManager) (LUCENE-10002)
+
+`IndexSearch#search(Query, Collector)` is now being deprecated in favor of `IndexSearcher#search(Query, CollectorManager)`,
+as `CollectorManager` implementation would allow taking advantage of intra-query concurrency via its map-reduce API design.
+To migrate, use a provided `CollectorManager` implementation that suits your use cases, or change your `Collector` implementation
+to follow the new API pattern. The straight forward approach would be to wrap the single-threaded `Collector` into a degenerate `CollectorManager`.
+
+For example
+```java
+public class WrappingCollectorManager implements CollectorManager<CustomCollector, List<Object>> {
Review Comment:
nit: maybe call it `CustomCollectorManager` to align the name with the name of the collector?
--
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] GITHUB-13218: Add migrate entry for Collector to CollectorManager migration [lucene]
Posted by "zacharymorn (via GitHub)" <gi...@apache.org>.
zacharymorn merged PR #13238:
URL: https://github.com/apache/lucene/pull/13238
--
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] GITHUB-13218: Add migrate entry for Collector to CollectorManager migration [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #13238:
URL: https://github.com/apache/lucene/pull/13238#discussion_r1546424925
##########
lucene/MIGRATE.md:
##########
@@ -185,6 +185,34 @@ enum.
`IOContext#LOAD` has been replaced with `IOContext#PRELOAD`.
+### IndexSearch#search(Query, Collector) being deprecated in favor of IndexSearcher#search(Query, CollectorManager) (LUCENE-10002)
+
+`IndexSearch#search(Query, Collector)` is now being deprecated in favor of `IndexSearcher#search(Query, CollectorManager)`,
Review Comment:
Maybe say "has been removed" instead of "is now being deprecated"? Since this is the `MIGRATE.md` for 10.0.0, won't (will?) we have removed the `Collector` based search API?
--
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] GITHUB-13218: Add migrate entry for Collector to CollectorManager migration [lucene]
Posted by "zacharymorn (via GitHub)" <gi...@apache.org>.
zacharymorn commented on code in PR #13238:
URL: https://github.com/apache/lucene/pull/13238#discussion_r1550291935
##########
lucene/MIGRATE.md:
##########
@@ -185,6 +185,34 @@ enum.
`IOContext#LOAD` has been replaced with `IOContext#PRELOAD`.
+### IndexSearch#search(Query, Collector) being deprecated in favor of IndexSearcher#search(Query, CollectorManager) (LUCENE-10002)
+
+`IndexSearch#search(Query, Collector)` is now being deprecated in favor of `IndexSearcher#search(Query, CollectorManager)`,
Review Comment:
Thanks Mike for the suggestion! I wonder if `has been deprecated` (instead of `has been removed`) would be better here, if for some reasons we are not able to completely delete the method and its usage by `10.0.0`? `deprecated` also sometimes imply `removed`, so we don't need to update this entry either if we are able to remove the method by `10.0.0`?
--
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] GITHUB-13218: Add migrate entry for Collector to CollectorManager migration [lucene]
Posted by "zacharymorn (via GitHub)" <gi...@apache.org>.
zacharymorn commented on code in PR #13238:
URL: https://github.com/apache/lucene/pull/13238#discussion_r1544688498
##########
lucene/MIGRATE.md:
##########
@@ -185,6 +185,34 @@ enum.
`IOContext#LOAD` has been replaced with `IOContext#PRELOAD`.
+### IndexSearch#search(Query, Collector) being deprecated in favor of IndexSearcher#search(Query, CollectorManager) (LUCENE-10002)
+
+`IndexSearch#search(Query, Collector)` is now being deprecated in favor of `IndexSearcher#search(Query, CollectorManager)`,
+as `CollectorManager` implementation would allow taking advantage of intra-query concurrency via its map-reduce API design.
+To migrate, use a provided `CollectorManager` implementation that suits your use cases, or change your `Collector` implementation
+to follow the new API pattern. The straight forward approach would be to wrap the single-threaded `Collector` into a degenerate `CollectorManager`.
+
+For example
+```java
+public class WrappingCollectorManager implements CollectorManager<CustomCollector, List<Object>> {
Review Comment:
Done.
--
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] GITHUB-13218: Add migrate entry for Collector to CollectorManager migration [lucene]
Posted by "zacharymorn (via GitHub)" <gi...@apache.org>.
zacharymorn commented on code in PR #13238:
URL: https://github.com/apache/lucene/pull/13238#discussion_r1552338688
##########
lucene/MIGRATE.md:
##########
@@ -185,6 +185,34 @@ enum.
`IOContext#LOAD` has been replaced with `IOContext#PRELOAD`.
+### IndexSearch#search(Query, Collector) being deprecated in favor of IndexSearcher#search(Query, CollectorManager) (LUCENE-10002)
+
+`IndexSearch#search(Query, Collector)` is now being deprecated in favor of `IndexSearcher#search(Query, CollectorManager)`,
Review Comment:
Sounds good, thanks Mike!
--
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] GITHUB-13218: Add migrate entry for Collector to CollectorManager migration [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on code in PR #13238:
URL: https://github.com/apache/lucene/pull/13238#discussion_r1551778848
##########
lucene/MIGRATE.md:
##########
@@ -185,6 +185,34 @@ enum.
`IOContext#LOAD` has been replaced with `IOContext#PRELOAD`.
+### IndexSearch#search(Query, Collector) being deprecated in favor of IndexSearcher#search(Query, CollectorManager) (LUCENE-10002)
+
+`IndexSearch#search(Query, Collector)` is now being deprecated in favor of `IndexSearcher#search(Query, CollectorManager)`,
Review Comment:
OK good point, let's just leave the wording as is?
--
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] GITHUB-13218: Add migrate entry for Collector to CollectorManager migration [lucene]
Posted by "zacharymorn (via GitHub)" <gi...@apache.org>.
zacharymorn commented on code in PR #13238:
URL: https://github.com/apache/lucene/pull/13238#discussion_r1544685493
##########
lucene/MIGRATE.md:
##########
@@ -185,6 +185,34 @@ enum.
`IOContext#LOAD` has been replaced with `IOContext#PRELOAD`.
+### IndexSearch#search(Query, Collector) being deprecated in favor of IndexSearcher#search(Query, CollectorManager) (LUCENE-10002)
+
+`IndexSearch#search(Query, Collector)` is now being deprecated in favor of `IndexSearcher#search(Query, CollectorManager)`,
+as `CollectorManager` implementation would allow taking advantage of intra-query concurrency via its map-reduce API design.
+To migrate, use a provided `CollectorManager` implementation that suits your use cases, or change your `Collector` implementation
+to follow the new API pattern. The straight forward approach would be to wrap the single-threaded `Collector` into a degenerate `CollectorManager`.
Review Comment:
Ah yes let me use a different word 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