You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/09/18 12:04:42 UTC
[GitHub] [lucene] stefanvodita opened a new pull request, #11780: GH#11601: Add ability to compute reader states after refresh
stefanvodita opened a new pull request, #11780:
URL: https://github.com/apache/lucene/pull/11780
This PR is only meant as a starting point for a conversation and not to be merged as is.
It is a proposal to let users retrieve new `SortedSedDocValuesReaderState` objects after doing a refresh in a single method call to the `ReferenceManager`. The new reader states will have updated ordinal maps.
Problems this solves:
1. Getting reader states that correspond to the current `IndexReader`. We ensure this by using the `refreshLock`.
2. Avoid a circular dependency from the core module on the facets module by using the `StateCalculator` functional interface.
--
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
[GitHub] [lucene] stefanvodita commented on pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on PR #11780:
URL: https://github.com/apache/lucene/pull/11780#issuecomment-1464991650
Thanks for the feedback @gmiller! The way you describe the state-of-the-world makes perfect sense to me, I think we’re on the same page there.
What I was trying to do with `SsdvReaderStatesManager` is similar to what you suggest. I definitely made that harder to get across by not removing code from old revisions, but I’ve fixed that now.
Highlights from the new revision:
1. In `FacetsConfig` I’ve made it so that we store and are able to recall which fields are used for faceting.
2. `SsdvReaderStatesManager` stores and retrieves reader states. You mentioned doing this in `FacetsConfig`, but it felt like that added a lot of new responsibility to the class, so I decided to keep it separate. Let me know if you still think it should go into `FacetsConfig`.
> we could consider a different ctor for SSDVFacets that takes a FacetsConfig instance
I didn’t do this part because AFAICT to make a call to a Facets implementation, the user needs to select a field. Right now, they do this by encapsulating the field in the reader state. If the user passed a `SsdvReaderStatesManager` instead of a reader state, they would also need to pass the field name. It seems easier to just pass the reader state.
> a single FacetsConfig instance could be used with different index readers while a refresh is happening
Interesting! I’ve accounted for this in `SsdvReaderStatesManager`, but haven’t yet implemented a “forgetting” mechanism for old `IndexReader`s. I think we can have at most 2 active `IndexReader`s, right? If so, we set something up along the lnes of `LiveFieldValues`, with an “old” map and a “current” map.
--
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
[GitHub] [lucene] gsmiller commented on pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #11780:
URL: https://github.com/apache/lucene/pull/11780#issuecomment-1464091101
Sorry @stefanvodita, just now coming back to this after a break.
OK, so current state-of-the-world:
Right now, users have to instantiate reader state instances for each field they're using for SSDV faceting so they can provide those to SSDFacets. These reader state instances must be re-created when the index is refreshed, and users have to manage this on their own. Creating these state instances is expensive, so users need a way to keep track of them, but also refresh them when necessary. To be fair though, users have all the capabilities they need to do efficient SSDV faceting, it's just a lot to keep track of for them (in my opinion anyway).
So I think the real question here is if we can do something that makes users' lives easier w.r.t. keeping track of these states and refreshing them when necessary. I think Mike's point-of-view is that maybe this isn't such a big deal and may not be necessary?
If we _do_ want to explore ways to make this interaction easier for users, I think `FacetsConfig` is the only realistic "hook" in some sense. One issue is that there is actually no current place where we can know what SSDV fields in the index are being used for faceting. We can't rely on the "dim config" exposed by FacetsConfig since users are not required to register any specific dim config for a faceting field if they're happy with the defaults.
I wonder if one option to explore is to have a `FacetsConfig` instance keep track of the SSDV fields being used for faceting when it builds docs? That's the one place we actually know that an SSDV field is being added for the purpose of faceting. If the `FacetsConfig` instance kept track of SSDV faceting fields in this way, we could add a helper method to `FacetsConfig` that takes an `IndexReader` as input and creates a new set of reader states. Then, we could consider a different ctor for SSDVFacets that takes a `FacetsConfig` instance instead of taking a state instance directly, and the `FacetsConfig` instance could provide the correct state instance. I think there's some complexity in here though since a single `FacetsConfig` instance could be used with different index readers while a refresh is happening, so it would probably need to keep track of a state instance for each SSDV field + IndexReader or something. I don't know. This might all just be too complicated to be worth i
t, but it would be simpler for users if all they had to do was provide their `FacetsConfig` instance to SSDVFacets and then call some method on that `FacetsConfig` instance whenever they refresh their index.
--
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
[GitHub] [lucene] stefanvodita commented on pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by GitBox <gi...@apache.org>.
stefanvodita commented on PR #11780:
URL: https://github.com/apache/lucene/pull/11780#issuecomment-1269159465
Thinking some more about this change in light of the comments, I see 3 ways it could go:
1. Do nothing. The user can already instantiate new reader states after a refresh if they want. No new API is required.
2. What this PR tried - offer an API to refresh and compute new reader states as one operation. @mikemccand points out this is likely not useful.
3. Allow the user to update the ordinal maps in the reader states they already have without requiring them to completely recreate the reader states. I’m not sure how much this accomplishes. The Javadoc suggests that the bulk of the work in instantiating a SortedSetDocValuesReaderState is building the ordinal map. In that case, updating the ordinal maps instead of recreating the reader states doesn’t have much benefit.
Do any of these seem like reasonable approaches to address the original issue? If not, how else could we approach this?
--
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
[GitHub] [lucene] stefanvodita commented on pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by GitBox <gi...@apache.org>.
stefanvodita commented on PR #11780:
URL: https://github.com/apache/lucene/pull/11780#issuecomment-1282371150
That makes sense. Maybe I'm not addressing the right problem. @gsmiller - as the issue's author, 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
[GitHub] [lucene] zhaih commented on pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by GitBox <gi...@apache.org>.
zhaih commented on PR #11780:
URL: https://github.com/apache/lucene/pull/11780#issuecomment-1258623624
For 1. I think we can always create a ReferenceManager of `IndexReaderAndState` so that those two are always refreshed together and you can acquire them together.
For 2. I think people are discussing about deprecating SearcherManager right now (because we're having a timeout per indexSearcher which makes it not reusable) so I think there's not much value in it, but I could be wrong..
--
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
[GitHub] [lucene] mikemccand commented on a diff in pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by GitBox <gi...@apache.org>.
mikemccand commented on code in PR #11780:
URL: https://github.com/apache/lucene/pull/11780#discussion_r983451389
##########
lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java:
##########
@@ -219,6 +219,36 @@ public final boolean maybeRefresh() throws IOException {
return doTryRefresh;
}
+ /** Compute some state of the reference using a parameter. */
+ @FunctionalInterface
+ public interface StateCalculator<R, G, P> {
+ R calculate(G current, P param);
+ }
+
+ /**
+ * If you need to compute something after the refresh, you can use this method instead of {@link
+ * #maybeRefresh()}.
+ *
+ * @throws IOException if refreshing the resource causes an {@link IOException}
+ */
+ public final <R, P> R maybeRefreshAndReturnState(
+ StateCalculator<R, G, P> refreshedStateCalculator, P param) throws IOException {
+ ensureOpen();
+
+ // Ensure only 1 thread does refresh at once; other threads just return immediately:
+ final boolean doTryRefresh = refreshLock.tryLock();
+ if (doTryRefresh) {
+ try {
+ doMaybeRefresh();
+ return refreshedStateCalculator.calculate(current, param);
Review Comment:
Yeah I don't think that's important to guard against?
An application typically has a separate refresh thread, and that (one) thread will call refresh, compute top-level data structures like `OrdinalMap`, SSDV states, etc., and then wait some time and refresh again. So the application can easily ensure it's not double-calling refresh, and if it might, it should use its own locking.
--
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
[GitHub] [lucene] gsmiller commented on a diff in pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on code in PR #11780:
URL: https://github.com/apache/lucene/pull/11780#discussion_r1132608504
##########
lucene/facet/src/java/org/apache/lucene/facet/sortedset/SSDVReaderStatesCalculator.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.facet.sortedset;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.stream.Collectors;
+import org.apache.lucene.facet.FacetsConfig;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.ReferenceManager;
+
+/**
+ * Get reader states after a refresh. Example call: readerStates =
+ * searcherManager.maybeRefreshAndReturnState(new SSDVReaderStatesCalculator(), config);
+ */
+public class SSDVReaderStatesCalculator
+ implements ReferenceManager.StateCalculator<
+ List<SortedSetDocValuesReaderState>, IndexSearcher, FacetsConfig> {
+
+ /** No arguments needed */
+ public SSDVReaderStatesCalculator() {}
+
+ @Override
+ public List<SortedSetDocValuesReaderState> calculate(IndexSearcher current, FacetsConfig config) {
+ return config.getDimConfigs().keySet().stream()
Review Comment:
One issue here is that the user may have indexed SSDVFacetFields without making any configuration changes (i.e., the default dim config was fine for them). In this case, the dim configs you reference here won't know about the fields.
--
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
[GitHub] [lucene] zhaih commented on pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by GitBox <gi...@apache.org>.
zhaih commented on PR #11780:
URL: https://github.com/apache/lucene/pull/11780#issuecomment-1257538633
How is it different from making a subclass/alternative of ReaderManager and just put the state calculation inside `refreshIfNeed`?
--
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
[GitHub] [lucene] stefanvodita commented on pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by GitBox <gi...@apache.org>.
stefanvodita commented on PR #11780:
URL: https://github.com/apache/lucene/pull/11780#issuecomment-1345547314
@mikemccand’s feedback made me shift focus from rebuilding ordinal maps on refresh to just making it convenient for the user to do so. `SsdvReaderStatesManager` shows how that might be 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
[GitHub] [lucene] mikemccand commented on pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by GitBox <gi...@apache.org>.
mikemccand commented on PR #11780:
URL: https://github.com/apache/lucene/pull/11780#issuecomment-1276316505
> 3\. Allow the user to update the ordinal maps in the reader states they already have without requiring them to completely recreate the reader states. I’m not sure how much this accomplishes. The Javadoc suggests that the bulk of the work in instantiating a SortedSetDocValuesReaderState is building the ordinal map. In that case, updating the ordinal maps instead of recreating the reader states doesn’t have much benefit.
I think this is not a great option (in-place update) because it'd break Lucene's point-in-time semantics. It would be nice to be able to create a new `OrdinalMap` for a refreshed reader by passing in the old `OrdinalMap` and being more "incremental" in how the new one is created (building on, but not altering, the old one)? But that is really a separate issue and is quite complex I think :)
> 1. Do nothing. The user can already instantiate new reader states after a refresh if they want. No new API is required.
+1 for this option -- I don't see why this is a problem for users/applications. No need to do this atomically under the refresh lock.
--
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
[GitHub] [lucene] stefanvodita commented on a diff in pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by GitBox <gi...@apache.org>.
stefanvodita commented on code in PR #11780:
URL: https://github.com/apache/lucene/pull/11780#discussion_r982630324
##########
lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java:
##########
@@ -219,6 +219,36 @@ public final boolean maybeRefresh() throws IOException {
return doTryRefresh;
}
+ /** Compute some state of the reference using a parameter. */
+ @FunctionalInterface
+ public interface StateCalculator<R, G, P> {
+ R calculate(G current, P param);
+ }
+
+ /**
+ * If you need to compute something after the refresh, you can use this method instead of {@link
+ * #maybeRefresh()}.
+ *
+ * @throws IOException if refreshing the resource causes an {@link IOException}
+ */
+ public final <R, P> R maybeRefreshAndReturnState(
+ StateCalculator<R, G, P> refreshedStateCalculator, P param) throws IOException {
+ ensureOpen();
+
+ // Ensure only 1 thread does refresh at once; other threads just return immediately:
+ final boolean doTryRefresh = refreshLock.tryLock();
+ if (doTryRefresh) {
+ try {
+ doMaybeRefresh();
+ return refreshedStateCalculator.calculate(current, param);
Review Comment:
We’re guaranteeing that no other refresh happens before computing the reader states. Maybe that’s not important to do? If so, then the user can always refresh and then compute their own reader states.
--
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
[GitHub] [lucene] stefanvodita commented on pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by GitBox <gi...@apache.org>.
stefanvodita commented on PR #11780:
URL: https://github.com/apache/lucene/pull/11780#issuecomment-1258609162
> How is it different from making a subclass/alternative of ReaderManager and just put the state calculation inside `refreshIfNeed`?
I think there’s two things that would be different. Please correct me if I’m wrong.
1. If we simply override `refreshIfNeeded`, we don’t have a way to return the new reader states. We would still need to create some new method that does the refresh and returns the reader states.
2. Doing this in the `ReferenceManager` makes it work for both `ReaderManager` and `SearcherManager` with the right `StateCalculator` for each. I don’t know how valuable that is though.
--
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
[GitHub] [lucene] mikemccand commented on a diff in pull request #11780: GH#11601: Add ability to compute reader states after refresh
Posted by GitBox <gi...@apache.org>.
mikemccand commented on code in PR #11780:
URL: https://github.com/apache/lucene/pull/11780#discussion_r982553986
##########
lucene/core/src/java/org/apache/lucene/search/ReferenceManager.java:
##########
@@ -219,6 +219,36 @@ public final boolean maybeRefresh() throws IOException {
return doTryRefresh;
}
+ /** Compute some state of the reference using a parameter. */
+ @FunctionalInterface
+ public interface StateCalculator<R, G, P> {
+ R calculate(G current, P param);
+ }
+
+ /**
+ * If you need to compute something after the refresh, you can use this method instead of {@link
+ * #maybeRefresh()}.
+ *
+ * @throws IOException if refreshing the resource causes an {@link IOException}
+ */
+ public final <R, P> R maybeRefreshAndReturnState(
+ StateCalculator<R, G, P> refreshedStateCalculator, P param) throws IOException {
+ ensureOpen();
+
+ // Ensure only 1 thread does refresh at once; other threads just return immediately:
+ final boolean doTryRefresh = refreshLock.tryLock();
+ if (doTryRefresh) {
+ try {
+ doMaybeRefresh();
+ return refreshedStateCalculator.calculate(current, param);
Review Comment:
Hmm why is it necessary to compute this state under lock? Can't the caller just compute the state themselves when they get the refreshed reader?
--
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