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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
