SOLR-17504: CoreContainer calls UpdateHandler.commit. #2786
Conversation
| @@ -28,6 +28,10 @@ public class TimeOut { | |||
| private final long timeoutAt, startTime; | |||
| private final TimeSource timeSource; | |||
|
|
|||
| public TimeOut(long intervalSeconds) { | |||
There was a problem hiding this comment.
just taking a long (a primitive) isn't particularly obvious; I recommend not adding this.
| * 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}. |
There was a problem hiding this 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.
There was a problem hiding this comment.
I suppose there should be an enum, searcher behavior:
- noSearcher
- realtimeSearcher
- standardSearcher
- standardSearcherNoWait
There was a problem hiding this comment.
If you agree, such a change would be a nice refactoring done first before this PR
There was a problem hiding this 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.
There was a problem hiding this 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.
| // todo: refactor this shared code (or figure out why a real CommitUpdateCommand can't | ||
| // be used) |
There was a problem hiding this comment.
I know you didn't write that comment but it's unclear to me what is meant.
There was a problem hiding this comment.
Yes, also unclear to me.
| @@ -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)) { | |||
There was a problem hiding this comment.
Really smart observation here.
| try { | ||
| if (iw != null) { | ||
| iw.commit(); | ||
| SolrQueryRequest req = new LocalSolrQueryRequest(core, new ModifiableSolrParams()); | ||
| CommitUpdateCommand cmd = |
There was a problem hiding this comment.
nit: "var" would be clear and short
There was a problem hiding this comment.
Maybe, I can change but I don't like var.
| // switch old core to readOnly | ||
| core.readOnly = true; |
There was a problem hiding this comment.
I suppose doing this before was to protect changes happening after the commit sneaking it. Why did you move it?
There was a problem hiding this 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.
There was a problem hiding this 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()
bruno-roustant
force-pushed
the
direct_update_handler
branch
from
3c35a87
to
8cafc88
Compare
|
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. |
| @@ -21,9 +21,21 @@ | |||
|
|
|||
| /** A commit index command encapsulated in an object. */ | |||
| public class CommitUpdateCommand extends UpdateCommand { | |||
|
|
|||
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.