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.

Sign up for GitHub

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

Jump to bottom

SOLR-15983: parallel log replay now uses separate UpdateRequestProcessor objects #601

Merged
merged 10 commits into from Feb 21, 2022
Merged

SOLR-15983: parallel log replay now uses separate UpdateRequestProcessor objects #601

merged 10 commits into from Feb 21, 2022

Conversation

cpoerschke
Copy link
Contributor

@cpoerschke cpoerschke mentioned this pull request Feb 7, 2022
Closed
@dsmiley
Copy link
Contributor

dsmiley commented Feb 9, 2022

This PR isn't clear to me. It appears it would ignore the existing created proc and simply create another. I don't get the point of that. AFAICT, this method (doReplay) runs in its own thread (from run()) as well.

@cpoerschke
Copy link
Contributor Author

This PR isn't clear to me. It appears it would ignore the existing created proc and simply create another. I don't get the point of that. AFAICT, this method (doReplay) runs in its own thread (from run()) as well.

Yes, the doReplay method runs in its own thread but since 6084da5 and https://issues.apache.org/jira/browse/SOLR-12338 it can parallelise some of the work via the executor i.e. additional threads; but only some of the work goes to the executor and therefore the existing proc continues to be used in the doReplay method's thread (and the executor threads get their own proc objects).

@dsmiley
Use a ThreadLocal of the URP to avoid re-creating and avoid excessive…
690dfab 
… finish() calls and to make it clearer that the URPs are used correctly (have correct lifecycle and aren't shared).
@dsmiley
Merge branch 'main' into SOLR-15983-c
27b5e3e
@dsmiley
Copy link
Contributor

dsmiley commented Feb 12, 2022

I found the lifecycle of these "proc" instances in this code to not be as clear as I'd like. And I saw that finish() was being called after each document (when parallel replay) which doesn't sound nice, (may cause excessive fsync). I also don't love that these proc's would then be created for each doc. So I fixed all these matters by using a ThreadLocal scoped to this processing to hold the proc. They need to be be kept in a list so they can all be finish()'ed and close()'ed at the end. WDYT? Tests pass.

@cpoerschke
Copy link
Contributor Author

I found the lifecycle of these "proc" instances in this code to not be as clear as I'd like. And I saw that finish() was being called after each document (when parallel replay) which doesn't sound nice, (may cause excessive fsync). I also don't love that these proc's would then be created for each doc. So I fixed all these matters by using a ThreadLocal scoped to this processing to hold the proc. They need to be be kept in a list so they can all be finish()'ed and close()'ed at the end. WDYT? Tests pass.

Very nice, thanks! The parallelised and non-parallelised proc having the same lifecycle is neat and also the on-demand creation handles the scenario where the non-parallelised proc is not required i.e. no DBQ replay.

cpoerschke
cpoerschke commented Feb 14, 2022
View reviewed changes
solr/CHANGES.txt
@@ -642,6 +642,8 @@ Bug Fixes

* SOLR-15558: Don't wait for zombie processes to exit when stopping. (Colvin Cowie)

* SOLR-15983: Fix ClassCastException in UpdateLog$LogReplayer.doReplay. (Christine Poerschke, David Smiley)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I've optimistically added the entry in the 9.0 bugfix section. Does that sound reasonable? The change(s) being #609 followed by #601 here.

cc: @janhoy

@cpoerschke
Copy link
Contributor Author

Re-ran the tests, they pass.

@cpoerschke cpoerschke merged commit 7c07670 into apache:main Feb 21, 2022
asfgit pushed a commit that referenced this pull request Feb 21, 2022
@cpoerschke
SOLR-15983: parallel log replay now uses separate UpdateRequestProces…
7d61190 
…sor objects (#601)

* parallel log replay now uses separate UpdateRequestProcessor objects

* executor local variable in UpdateLog.doReplay can be null

* Use a ThreadLocal of the URP to avoid re-creating and avoid excessive finish() calls and to make it clearer that the URPs are used correctly (have correct lifecycle and aren't shared).

* remove unused procThreadLocal member in favour of local variable with same name

* just one for-loop on procPool to finish-and-close

* add solr/CHANGES.txt entry

Co-authored-by: David Smiley <dsmiley@salesforce.com>
(cherry picked from commit 7c07670)
asfgit pushed a commit that referenced this pull request Feb 24, 2022
@cpoerschke
SOLR-15983: parallel log replay now uses separate UpdateRequestProces…
bc501ba 
…sor objects (#601)

* parallel log replay now uses separate UpdateRequestProcessor objects

* executor local variable in UpdateLog.doReplay can be null

* Use a ThreadLocal of the URP to avoid re-creating and avoid excessive finish() calls and to make it clearer that the URPs are used correctly (have correct lifecycle and aren't shared).

* remove unused procThreadLocal member in favour of local variable with same name

* just one for-loop on procPool to finish-and-close

* add solr/CHANGES.txt entry

Co-authored-by: David Smiley <dsmiley@salesforce.com>
(cherry picked from commit 7c07670)
(cherry picked from commit 7d61190)

Resolved Conflicts:
	solr/CHANGES.txt
	solr/core/src/java/org/apache/solr/update/UpdateLog.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsmiley dsmiley dsmiley approved these changes

@CaoManhDat CaoManhDat Awaiting requested review from CaoManhDat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@cpoerschke @dsmiley