dsmiley commented on code in PR #2631:
URL: https://github.com/apache/solr/pull/2631#discussion_r1712555039
##########
solr/core/src/java/org/apache/solr/update/processor/SkipExistingDocumentsProcessorFactory.java:
##########
@@ -264,11 +268,23 @@ public void processAdd(AddUpdateCommand cmd) throws
IOException {
return;
}
- if (skipUpdateIfMissing && isUpdate && isLeader(cmd) &&
!doesDocumentExist(indexedDocId)) {
- if (log.isDebugEnabled()) {
- log.debug("Skipping update to non-existent document ID {}",
indexedDocId.utf8ToString());
+ if (skipUpdateIfMissing && isUpdate && isLeader(cmd)) {
+ if (!cmd.getSelfOrNestedDocIdStr().equals(cmd.getIndexedIdStr())) {
+ if (!doesChildDocumentExist(cmd)) {
+ if (log.isDebugEnabled()) {
+ log.debug(
+ "Skipping update to non-existent child document ID {}",
+ cmd.getSelfOrNestedDocIdStr());
+ }
+ return;
+ }
+ } else if (!doesDocumentExist(indexedDocId)) {
+ if (log.isDebugEnabled()) {
+ log.debug(
+ "Skipping update to non-existent document ID {}",
indexedDocId.utf8ToString());
Review Comment:
use cmd.getPrintableId instead of utf8ToString
##########
solr/core/src/test/org/apache/solr/update/processor/SkipExistingDocumentsProcessorFactoryTest.java:
##########
@@ -314,6 +314,22 @@ public void
testSkippableUpdateIsSkippedIfSkipUpdatesTrue() throws IOException {
verify(next, never()).processAdd(cmd);
}
+ @Test
Review Comment:
I have faith that this unit test works so it's good enough for me (don't
need the expense of a higher level test). It's easy to read too.
Presumably you've checked that the test would fail were it not for your
changes?
##########
solr/core/src/java/org/apache/solr/update/processor/SkipExistingDocumentsProcessorFactory.java:
##########
@@ -264,11 +268,23 @@ public void processAdd(AddUpdateCommand cmd) throws
IOException {
return;
}
- if (skipUpdateIfMissing && isUpdate && isLeader(cmd) &&
!doesDocumentExist(indexedDocId)) {
- if (log.isDebugEnabled()) {
- log.debug("Skipping update to non-existent document ID {}",
indexedDocId.utf8ToString());
+ if (skipUpdateIfMissing && isUpdate && isLeader(cmd)) {
+ if (!cmd.getSelfOrNestedDocIdStr().equals(cmd.getIndexedIdStr())) {
+ if (!doesChildDocumentExist(cmd)) {
Review Comment:
a single comment above saying "must be a child doc" would be helpful
##########
solr/core/src/java/org/apache/solr/update/processor/SkipExistingDocumentsProcessorFactory.java:
##########
@@ -264,11 +268,23 @@ public void processAdd(AddUpdateCommand cmd) throws
IOException {
return;
}
- if (skipUpdateIfMissing && isUpdate && isLeader(cmd) &&
!doesDocumentExist(indexedDocId)) {
- if (log.isDebugEnabled()) {
- log.debug("Skipping update to non-existent document ID {}",
indexedDocId.utf8ToString());
+ if (skipUpdateIfMissing && isUpdate && isLeader(cmd)) {
+ if (!cmd.getSelfOrNestedDocIdStr().equals(cmd.getIndexedIdStr())) {
+ if (!doesChildDocumentExist(cmd)) {
+ if (log.isDebugEnabled()) {
+ log.debug(
+ "Skipping update to non-existent child document ID {}",
+ cmd.getSelfOrNestedDocIdStr());
+ }
+ return;
+ }
+ } else if (!doesDocumentExist(indexedDocId)) {
Review Comment:
a single comment at EOL saying "not a child doc" would be helpful
--
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]
