You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "gerlowskija (via GitHub)" <gi...@apache.org> on 2024/10/15 13:34:07 UTC
[PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
gerlowskija opened a new pull request, #2765:
URL: https://github.com/apache/solr/pull/2765
# Description
SolrQueryRequest is a non-threadsafe type, but was being shared across executor threads during UpdateLog replay. This introduces a number of issues, not the least being a ConcurrentModificationException if multiple threads happen to tweak the request 'context' simultaneously.
# Solution
This commit fixes this issue by giving each executor thread a unique LocalSolrQueryRequest instance to use.
# Tests
The problem is reproducible (on my hardware at least) via 'beasting' `BasicDistributedZkTest` for several hundred iterations but is otherwise hard to trigger. The test passes reliably with this fix, which can give us some confidence I think. Though if anyone can think of a better way to capture this in an automated test, I'd love to get some additional validation!
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/solr/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended, not available for branches on forks living under an organisation)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija merged PR #2765:
URL: https://github.com/apache/solr/pull/2765
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #2765:
URL: https://github.com/apache/solr/pull/2765#issuecomment-2420002345
Sure - feel free to push to the branch if you've got an idea.
Not totally sure what you mean by a "re-using pool", but I'm sure that'll be clearer on seeing the code. (Unless it's a large enough change that it's worth going through in detail before attempting the code? Up to you...)
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2765:
URL: https://github.com/apache/solr/pull/2765#discussion_r1805481104
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -2097,10 +2098,19 @@ public void doReplay(TransactionLog translog) {
UpdateRequestProcessorChain processorChain = req.getCore().getUpdateProcessingChain(null);
Collection<UpdateRequestProcessor> procPool =
Collections.synchronizedList(new ArrayList<>());
+ final var params =
+ new MapSolrParams(
+ Map.of(
+ DISTRIB_UPDATE_PARAM,
+ FROMLEADER.toString(),
+ DistributedUpdateProcessor.LOG_REPLAY,
+ "true"));
Review Comment:
This is duplicative with other code in this class; it should be factored out. You can just refer to the very same params of `req`, even use the same immutable variant you just coded here for that.
I started looking at why the duplication is there, and I see we create a single req & rsp for the replay as a whole that will be separate from the LocalSolrRequest in the ThreadLocal. The UpdateCommand is created from the parent request but the URP is created from the local request. That feels wrong even if it works. Admittedly it's probably too much scope to refactor (I started locally and it's not trivial).
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #2765:
URL: https://github.com/apache/solr/pull/2765#discussion_r1803271038
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -2097,10 +2098,19 @@ public void doReplay(TransactionLog translog) {
UpdateRequestProcessorChain processorChain = req.getCore().getUpdateProcessingChain(null);
Collection<UpdateRequestProcessor> procPool =
Collections.synchronizedList(new ArrayList<>());
+ final var params =
+ new MapSolrParams(
+ Map.of(
+ DISTRIB_UPDATE_PARAM,
+ FROMLEADER.toString(),
+ DistributedUpdateProcessor.LOG_REPLAY,
+ "true"));
ThreadLocal<UpdateRequestProcessor> procThreadLocal =
ThreadLocal.withInitial(
() -> {
- var proc = processorChain.createProcessor(req, rsp);
+ // SolrQueryRequest is not thread-safe, so use a copy when creating URPs
+ final var solrQueryRequest = new LocalSolrQueryRequest(uhandler.core, params);
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on PR #2765:
URL: https://github.com/apache/solr/pull/2765#issuecomment-2417910950
I'd like to contribute to this PR to replace the ThreadLocal with a re-using pool that we mostly already have.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #2765:
URL: https://github.com/apache/solr/pull/2765#issuecomment-2414298119
Is the not-anymore-used `req` object still used somewhere else? And we are sure there aren't other settings that live in that request?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #2765:
URL: https://github.com/apache/solr/pull/2765#issuecomment-2416735316
> Is the not-anymore-used req object still used somewhere else?
Yes, actually! Meant to call this out in a comment here or in the code - the "original" LocalSolrQueryRequest is technically still accessible on the UpdateCommand seen by individual processors. So there's still the possibility this object could be messed with by multiple threads.
That said - I don't think this is a risk in practice. I spent a good bit of time tracing through all of the `UpdateCommand.getReq()` invocations, and they're all "read only" calls to get either the SolrCore or the IndexSchema relevant to this request. (Both of which are commonly used across threads.)
Both from this and from the problem David described above, it's clear that the UpdateLog replay threading could use a larger rethink, but I'm reluctant to dive into that 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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on code in PR #2765:
URL: https://github.com/apache/solr/pull/2765#discussion_r1819471160
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -2097,10 +2098,19 @@ public void doReplay(TransactionLog translog) {
UpdateRequestProcessorChain processorChain = req.getCore().getUpdateProcessingChain(null);
Collection<UpdateRequestProcessor> procPool =
Collections.synchronizedList(new ArrayList<>());
+ final var params =
+ new MapSolrParams(
+ Map.of(
+ DISTRIB_UPDATE_PARAM,
+ FROMLEADER.toString(),
+ DistributedUpdateProcessor.LOG_REPLAY,
+ "true"));
Review Comment:
> The UpdateCommand is created from the parent request but the URP is created from the local request
Yeah, agreed. I also tried a larger refactor of the SolrQueryRequest/UpdateCommand boundary, but ultimately landed on it being "too much scope" to take on at this point.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2765:
URL: https://github.com/apache/solr/pull/2765#discussion_r1801734297
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -2097,10 +2098,19 @@ public void doReplay(TransactionLog translog) {
UpdateRequestProcessorChain processorChain = req.getCore().getUpdateProcessingChain(null);
Collection<UpdateRequestProcessor> procPool =
Collections.synchronizedList(new ArrayList<>());
+ final var params =
+ new MapSolrParams(
+ Map.of(
+ DISTRIB_UPDATE_PARAM,
+ FROMLEADER.toString(),
+ DistributedUpdateProcessor.LOG_REPLAY,
+ "true"));
ThreadLocal<UpdateRequestProcessor> procThreadLocal =
ThreadLocal.withInitial(
() -> {
- var proc = processorChain.createProcessor(req, rsp);
+ // SolrQueryRequest is not thread-safe, so use a copy when creating URPs
+ final var solrQueryRequest = new LocalSolrQueryRequest(uhandler.core, params);
Review Comment:
I'd call this var localRequest -- "local" being an important part.
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-17413: ulog replay should copy SolrQueryRequest [solr]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #2765:
URL: https://github.com/apache/solr/pull/2765#issuecomment-2420002865
Oops meant to tag you - @dsmiley
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org