You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "bruno-roustant (via GitHub)" <gi...@apache.org> on 2024/10/21 15:12:51 UTC
[PR] SOLR-17504: CoreContainer calls UpdateHandler.commit. [solr]
bruno-roustant opened a new pull request, #2786:
URL: https://github.com/apache/solr/pull/2786
https://issues.apache.org/jira/browse/SOLR-17504
# Description
This proposal is about unifying calls to UpdateHandler.commit.
CoreContainer.reload may call directly IndexWriter.commit if the core to reload is readonly (property of the Collection). This is an issue because it bypasses some logic in UpdateHandler.commit. In addition, the current code commits without taking the commit lock.
DirectUpdateHandler2.closeWriter may commit if the update log is not empty (not committed yet). But it does not call DirectUpdateHandler2.commit(), it copies commit() code instead for some reasons (comments in the code mention test failures).
# Solution
Make CoreContainer.reload call UpdateHandler.commit.
Add the same call to DirectUpdateHandler2.shouldCommit() as in the commit() method, otherwise commit metadata are lost. In addition shouldCommit() can be extended correctly, for example in the solr-sandbox encryption module which needs to intercept calls to shouldCommit().
# Tests
A new DirectUpdateHandlerWithUpdateLogTest is added to test specifically that the UpdateHandler.shouldCommit hook is called. It's a new test because it requires the update log to test the UpdateHandler.closeWriter logic, but DirectUpdateHandlerTest does not support the update log.
--
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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1810654297
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2061,13 +2066,16 @@ public void reload(String name, UUID coreId) {
RefCounted<IndexWriter> iwRef = core.getSolrCoreState().getIndexWriter(null);
if (iwRef != null) {
IndexWriter iw = iwRef.get();
- // switch old core to readOnly
- core.readOnly = true;
Review Comment:
Hum, good point. I moved it because I didn't realize your point and UpdateHanlder.commit() checks the code is not readonly, otherwise stops with an exception.
Actually, with your nice idea of an enum in CommitUpdateCommand, the intent would be clear and commit() would not reject if the core is closed after the commit. And we could restore the core.readonly = true at the right location 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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1809192524
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2061,13 +2066,16 @@ public void reload(String name, UUID coreId) {
RefCounted<IndexWriter> iwRef = core.getSolrCoreState().getIndexWriter(null);
if (iwRef != null) {
IndexWriter iw = iwRef.get();
- // switch old core to readOnly
- core.readOnly = true;
try {
if (iw != null) {
- iw.commit();
+ SolrQueryRequest req = new LocalSolrQueryRequest(core, new ModifiableSolrParams());
+ CommitUpdateCommand cmd =
Review Comment:
nit: "var" would be clear and short
--
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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1809171399
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -971,8 +971,10 @@ public void closeWriter(IndexWriter writer) throws IOException {
// todo: refactor this shared code (or figure out why a real CommitUpdateCommand can't
// be used)
Review Comment:
I know you didn't write that comment but it's unclear to me what is meant.
##########
solr/core/src/java/org/apache/solr/util/TimeOut.java:
##########
@@ -28,6 +28,10 @@ public class TimeOut {
private final long timeoutAt, startTime;
private final TimeSource timeSource;
+ public TimeOut(long intervalSeconds) {
Review Comment:
just taking a `long` (a primitive) isn't particularly obvious; I recommend not adding this.
##########
solr/core/src/test/org/apache/solr/update/DirectUpdateHandlerWithUpdateLogTest.java:
##########
Review Comment:
This test is beautiful
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -786,7 +786,7 @@ public void commit(CommitUpdateCommand cmd) throws IOException {
if (ulog != null) ulog.preSoftCommit(cmd);
if (cmd.openSearcher) {
core.getSearcher(true, false, waitSearcher);
- } else {
+ } else if (!(cmd instanceof ClosingCommitUpdateCommand)) {
Review Comment:
Really smart observation here.
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -1115,4 +1117,23 @@ public CommitTracker getCommitTracker() {
public CommitTracker getSoftCommitTracker() {
return softCommitTracker;
}
+
+ /**
+ * The purpose of this {@link CommitUpdateCommand} extension is to indicate that the {@link
+ * #commit(CommitUpdateCommand)} caller is closing the core and does not want to open any
+ * searcher. Because even if {@link CommitUpdateCommand#openSearcher} is false, a new real-time
+ * searcher will be opened with a standard {@link CommitUpdateCommand}. This internal {@link
+ * ClosingCommitUpdateCommand} is a way to not add another public flag to {@link
+ * CommitUpdateCommand}.
Review Comment:
And why would that be bad? My first reaction is, why is Bruno subclassing when this seems like another boolean on a commit. You could add a factory method for this scenario -- closeOnCommit so we know exactly what this looks like.
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -1115,4 +1117,23 @@ public CommitTracker getCommitTracker() {
public CommitTracker getSoftCommitTracker() {
return softCommitTracker;
}
+
+ /**
+ * The purpose of this {@link CommitUpdateCommand} extension is to indicate that the {@link
+ * #commit(CommitUpdateCommand)} caller is closing the core and does not want to open any
+ * searcher. Because even if {@link CommitUpdateCommand#openSearcher} is false, a new real-time
+ * searcher will be opened with a standard {@link CommitUpdateCommand}. This internal {@link
+ * ClosingCommitUpdateCommand} is a way to not add another public flag to {@link
+ * CommitUpdateCommand}.
Review Comment:
I suppose there should be an enum, searcher behavior:
* noSearcher
* realtimeSearcher
* standardSearcher
* standardSearcherNoWait
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -1115,4 +1117,23 @@ public CommitTracker getCommitTracker() {
public CommitTracker getSoftCommitTracker() {
return softCommitTracker;
}
+
+ /**
+ * The purpose of this {@link CommitUpdateCommand} extension is to indicate that the {@link
+ * #commit(CommitUpdateCommand)} caller is closing the core and does not want to open any
+ * searcher. Because even if {@link CommitUpdateCommand#openSearcher} is false, a new real-time
+ * searcher will be opened with a standard {@link CommitUpdateCommand}. This internal {@link
+ * ClosingCommitUpdateCommand} is a way to not add another public flag to {@link
+ * CommitUpdateCommand}.
Review Comment:
If you agree, such a change would be a nice refactoring done first before this PR
--
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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1810647477
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -971,8 +971,10 @@ public void closeWriter(IndexWriter writer) throws IOException {
// todo: refactor this shared code (or figure out why a real CommitUpdateCommand can't
// be used)
Review Comment:
Yes, also unclear 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@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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1809193371
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2061,13 +2066,16 @@ public void reload(String name, UUID coreId) {
RefCounted<IndexWriter> iwRef = core.getSolrCoreState().getIndexWriter(null);
if (iwRef != null) {
IndexWriter iw = iwRef.get();
- // switch old core to readOnly
- core.readOnly = true;
Review Comment:
I suppose doing this before was to protect changes happening after the commit sneaking it. Why did you move it?
--
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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant merged PR #2786:
URL: https://github.com/apache/solr/pull/2786
--
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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1810649307
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2061,13 +2066,16 @@ public void reload(String name, UUID coreId) {
RefCounted<IndexWriter> iwRef = core.getSolrCoreState().getIndexWriter(null);
if (iwRef != null) {
IndexWriter iw = iwRef.get();
- // switch old core to readOnly
- core.readOnly = true;
try {
if (iw != null) {
- iw.commit();
+ SolrQueryRequest req = new LocalSolrQueryRequest(core, new ModifiableSolrParams());
+ CommitUpdateCommand cmd =
Review Comment:
Maybe, I can change but I don't like var.
--
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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on PR #2786:
URL: https://github.com/apache/solr/pull/2786#issuecomment-2483275233
I tried to refactor to have an enum in CommitUpdateCommand instead of 3 booleans, but the change in callings classes was becoming large and odd with dependent classes. I finally reverted to keep the 2 booleans and add a private 3rd one. There is no refactoring, but it keeps clarity. Tell me what 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@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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1810654297
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2061,13 +2066,16 @@ public void reload(String name, UUID coreId) {
RefCounted<IndexWriter> iwRef = core.getSolrCoreState().getIndexWriter(null);
if (iwRef != null) {
IndexWriter iw = iwRef.get();
- // switch old core to readOnly
- core.readOnly = true;
Review Comment:
Hum, good point. I moved it because I didn't realize your point and UpdateHanlder.commit() checks the core is not readonly, otherwise stops with an exception.
Actually, with your nice idea of an enum in CommitUpdateCommand, the intent would be clear and commit() would not reject if the core is closed after the commit. And we could restore the core.readonly = true at the right location 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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1811150504
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -1115,4 +1117,23 @@ public CommitTracker getCommitTracker() {
public CommitTracker getSoftCommitTracker() {
return softCommitTracker;
}
+
+ /**
+ * The purpose of this {@link CommitUpdateCommand} extension is to indicate that the {@link
+ * #commit(CommitUpdateCommand)} caller is closing the core and does not want to open any
+ * searcher. Because even if {@link CommitUpdateCommand#openSearcher} is false, a new real-time
+ * searcher will be opened with a standard {@link CommitUpdateCommand}. This internal {@link
+ * ClosingCommitUpdateCommand} is a way to not add another public flag to {@link
+ * CommitUpdateCommand}.
Review Comment:
Definitely merely an internal change in terms of just the CommitUpdateCommand internals (I'm aware its fields are all public :-(. No backwards-compatibility concern.
Of course users could pass those booleans at an HTTP params level and it still needs to work, ultimately being converted to the appropriate enum. Not sure if you want to add a new http param for the enum as an alternative... I suppose you might.
--
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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1815544031
##########
solr/core/src/java/org/apache/solr/core/CoreContainer.java:
##########
@@ -2061,13 +2066,16 @@ public void reload(String name, UUID coreId) {
RefCounted<IndexWriter> iwRef = core.getSolrCoreState().getIndexWriter(null);
if (iwRef != null) {
IndexWriter iw = iwRef.get();
- // switch old core to readOnly
- core.readOnly = true;
Review Comment:
as an aside, I don't like that CoreContainer is doing SolrCore internal manipulations... like this should be a method on SolrCore like core.commit()
--
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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "bruno-roustant (via GitHub)" <gi...@apache.org>.
bruno-roustant commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1810646341
##########
solr/core/src/java/org/apache/solr/update/DirectUpdateHandler2.java:
##########
@@ -1115,4 +1117,23 @@ public CommitTracker getCommitTracker() {
public CommitTracker getSoftCommitTracker() {
return softCommitTracker;
}
+
+ /**
+ * The purpose of this {@link CommitUpdateCommand} extension is to indicate that the {@link
+ * #commit(CommitUpdateCommand)} caller is closing the core and does not want to open any
+ * searcher. Because even if {@link CommitUpdateCommand#openSearcher} is false, a new real-time
+ * searcher will be opened with a standard {@link CommitUpdateCommand}. This internal {@link
+ * ClosingCommitUpdateCommand} is a way to not add another public flag to {@link
+ * CommitUpdateCommand}.
Review Comment:
I like this enum idea very much.
If we replace two public boolean fields by an enum field, would it be a public API change, so limited to main branch?
Ideally, I would like to have this change in 9.x to get it in solr-sandbox.
--
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-17504: CoreContainer calls UpdateHandler.commit. [solr]
Posted by "dsmiley (via GitHub)" <gi...@apache.org>.
dsmiley commented on code in PR #2786:
URL: https://github.com/apache/solr/pull/2786#discussion_r1847720351
##########
solr/core/src/java/org/apache/solr/update/CommitUpdateCommand.java:
##########
@@ -21,9 +21,21 @@
/** A commit index command encapsulated in an object. */
public class CommitUpdateCommand extends UpdateCommand {
+
Review Comment:
Awesome docs!
--
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