[
https://issues.apache.org/jira/browse/SOLR-14413?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17179702#comment-17179702
]
Bram Van Dam edited comment on SOLR-14413 at 8/18/20, 3:31 PM:
---------------------------------------------------------------
[~epugh] I created a little test case in CursorPagingTest.java to demonstrate
the two parameters working together, but in doing so I appear to have done the
opposite. The test fails roughly 2/3 of the time. There appear to be three
failure modes:
# nextCursorMark is null (when it should be the same as the previous cursor
mark, or possibly a new mark)
# partialResults header is missing (this only seems to happen when the response
does not contain any results)
# not all documents are hit even after the cursor has reached its end (this one
occurs much less frequently)
When the assertions checking failure mode 1 and 2 are disabled, the test only
fails 1 time out of 4, when failure more 3 is hit. IMO only the third failure
mode is unacceptable, because it produces incorrect results.
The test never fails when minimum timeAllowed is set to 50ms or above. Values
that low are obviously ridiculous. Perhaps the documentation (or the
implementation?) should be updated to discourage unreasonably low values.
I'll attach an updated version of [~slackhappy]'s patch which includes my test
case. And I'll paste the test case here for good measure. Apologies for the
mess, I'm not familiar with Solr's test harness, so I'm sure there cleaner ways
of doing this.
{code:java}
/**
* - check whether the cursor can be advanced when using timeAllowed
* - ensure partial results are advertised as such
* - check the correctness of those results
*/
@SuppressWarnings({"unchecked", "rawtypes"})
public void testTimeAllowedAdvancesCursor() throws Exception {
String cursorMark;
ModifiableSolrParams params = null;
// Add 1000 docs, anything less and the requests complete too quickly to be
interesting
for(int i=1;i<=100;i++) {
// don't add in order of any field to ensure we aren't inadvertently
// counting on internal docid ordering
assertU(adoc("id", i+ "9", "str", "c", "float", "-3.2", "int", "42" + i));
assertU(adoc("id", i+ "7", "str", "c", "float", "-3.2", "int", "-1976" +
i));
assertU(adoc("id", i+ "2", "str", "c", "float", "-3.2", "int", "666" +
i));
assertU(adoc("id", i+ "0", "str", "b", "float", "64.5", "int", "-42" +
i));
assertU(adoc("id", i+ "5", "str", "b", "float", "64.5", "int", "2001" +
i));
assertU(adoc("id", i+ "8", "str", "b", "float", "64.5", "int", "4055" +
i));
assertU(adoc("id", i+ "6", "str", "a", "float", "64.5", "int", "7" + i));
assertU(adoc("id", i+ "1", "str", "a", "float", "64.5", "int", "7" + i));
assertU(adoc("id", i+ "4", "str", "a", "float", "11.1", "int", "6" + i));
assertU(adoc("id", i+ "3", "str", "a", "float", "11.1")); // int is
missing
}
assertU(commit());
// Prepare a list (sorted) of docIds we expect to find, populated without
using a cursor or timeAllowed
List<String> expectedDocIds = new ArrayList<>();
Map docResponse = (Map) fromJSONString(h.query(req(params("q", "-str:b",
"rows", "700", "fl", "id", "sort", "id desc"))));
expectedDocIds.addAll((Collection<? extends String>)
(((Map)docResponse.get("response")).get("docs")));
cursorMark = CURSOR_MARK_START;
params = params("q", "-str:b",
"rows","40",
"fl", "id",
"sort", "id desc");
List<String> foundDocIds = new ArrayList<>();
boolean cursorAdvanced = false;
do {
cursorAdvanced = false;
// If the cursor does not advance, we increase timeAllowed until it's
long enough to return a result
for(int timeAllowed=1; timeAllowed<=100; timeAllowed++) { // Keep
timeAllowed between 1 and 100ms
String json = assertJQ(req(params, CURSOR_MARK_PARAM, cursorMark,
CommonParams.TIME_ALLOWED, "" + timeAllowed));
Map response = (Map) fromJSONString(json);
String next = (String)response.get(CURSOR_MARK_NEXT);
assertNotNull(CURSOR_MARK_NEXT + " is null", next);
if(null != next && !cursorMark.equals(next)) {
// Cursor advanced, record foundDocs and move on
foundDocIds.addAll((Collection<? extends String>)
(((Map)response.get("response")).get("docs")));
cursorMark = next;
cursorAdvanced = true;
break;
} else if(foundDocIds.size() != 700) {
// Unless we've found all documents, the result must be partial
assertNull("Response is missing partialResults header for: "
+ "old cursor " + cursorMark + " new cursor: " + next + "
timeAllowed " + timeAllowed + " foundDocIds: " + foundDocIds.size(),
JSONTestUtil.match(json, "/responseHeader/partialResults==true",
JSONTestUtil.DEFAULT_DELTA));
}
}
} while(cursorAdvanced);
assertEquals("Should have found 700 documents eventually", expectedDocIds,
foundDocIds);
}
{code}
was (Author: bvd):
[~epugh] I created a little test case to demonstrate the two parameters working
together, but in doing so I appear to have done the opposite. The test fails
roughly 2/3 of the time. There appear to be three failure modes:
# nextCursorMark is null (when it should be the same as the previous cursor
mark, or possibly a new mark)
# partialResults header is missing (this only seems to happen when the response
does not contain any results)
# not all documents are hit even after the cursor has reached its end (this one
occurs much less frequently)
When the assertions checking failure mode 1 and 2 are disabled, the test only
fails 1 time out of 4, when failure more 3 is hit. IMO only the third failure
mode is unacceptable, because it produces incorrect results.
The test never fails when minimum timeAllowed is set to 50ms or above. Values
that low are obviously ridiculous. Perhaps the documentation (or the
implementation?) should be updated to discourage unreasonably low values.
I'll attach an updated version of [~slackhappy]'s patch which includes my test
case. And I'll paste the test case here for good measure. Apologies for the
mess, I'm not familiar with Solr's test harness, so I'm sure there cleaner ways
of doing this.
{code:java}
/**
* - check whether the cursor can be advanced when using timeAllowed
* - ensure partial results are advertised as such
* - check the correctness of those results
*/
@SuppressWarnings({"unchecked", "rawtypes"})
public void testTimeAllowedAdvancesCursor() throws Exception {
String cursorMark;
ModifiableSolrParams params = null;
// Add 1000 docs, anything less and the requests complete too quickly to be
interesting
for(int i=1;i<=100;i++) {
// don't add in order of any field to ensure we aren't inadvertently
// counting on internal docid ordering
assertU(adoc("id", i+ "9", "str", "c", "float", "-3.2", "int", "42" + i));
assertU(adoc("id", i+ "7", "str", "c", "float", "-3.2", "int", "-1976" +
i));
assertU(adoc("id", i+ "2", "str", "c", "float", "-3.2", "int", "666" +
i));
assertU(adoc("id", i+ "0", "str", "b", "float", "64.5", "int", "-42" +
i));
assertU(adoc("id", i+ "5", "str", "b", "float", "64.5", "int", "2001" +
i));
assertU(adoc("id", i+ "8", "str", "b", "float", "64.5", "int", "4055" +
i));
assertU(adoc("id", i+ "6", "str", "a", "float", "64.5", "int", "7" + i));
assertU(adoc("id", i+ "1", "str", "a", "float", "64.5", "int", "7" + i));
assertU(adoc("id", i+ "4", "str", "a", "float", "11.1", "int", "6" + i));
assertU(adoc("id", i+ "3", "str", "a", "float", "11.1")); // int is
missing
}
assertU(commit());
// Prepare a list (sorted) of docIds we expect to find, populated without
using a cursor or timeAllowed
List<String> expectedDocIds = new ArrayList<>();
Map docResponse = (Map) fromJSONString(h.query(req(params("q", "-str:b",
"rows", "700", "fl", "id", "sort", "id desc"))));
expectedDocIds.addAll((Collection<? extends String>)
(((Map)docResponse.get("response")).get("docs")));
cursorMark = CURSOR_MARK_START;
params = params("q", "-str:b",
"rows","40",
"fl", "id",
"sort", "id desc");
List<String> foundDocIds = new ArrayList<>();
boolean cursorAdvanced = false;
do {
cursorAdvanced = false;
// If the cursor does not advance, we increase timeAllowed until it's
long enough to return a result
for(int timeAllowed=1; timeAllowed<=100; timeAllowed++) { // Keep
timeAllowed between 1 and 100ms
String json = assertJQ(req(params, CURSOR_MARK_PARAM, cursorMark,
CommonParams.TIME_ALLOWED, "" + timeAllowed));
Map response = (Map) fromJSONString(json);
String next = (String)response.get(CURSOR_MARK_NEXT);
assertNotNull(CURSOR_MARK_NEXT + " is null", next);
if(null != next && !cursorMark.equals(next)) {
// Cursor advanced, record foundDocs and move on
foundDocIds.addAll((Collection<? extends String>)
(((Map)response.get("response")).get("docs")));
cursorMark = next;
cursorAdvanced = true;
break;
} else if(foundDocIds.size() != 700) {
// Unless we've found all documents, the result must be partial
assertNull("Response is missing partialResults header for: "
+ "old cursor " + cursorMark + " new cursor: " + next + "
timeAllowed " + timeAllowed + " foundDocIds: " + foundDocIds.size(),
JSONTestUtil.match(json, "/responseHeader/partialResults==true",
JSONTestUtil.DEFAULT_DELTA));
}
}
} while(cursorAdvanced);
assertEquals("Should have found 700 documents eventually", expectedDocIds,
foundDocIds);
}
{code}
> allow timeAllowed and cursorMark parameters
> -------------------------------------------
>
> Key: SOLR-14413
> URL: https://issues.apache.org/jira/browse/SOLR-14413
> Project: Solr
> Issue Type: Improvement
> Components: search
> Reporter: John Gallagher
> Priority: Minor
> Attachments: SOLR-14413.patch, timeallowed_cursormarks_results.txt
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Ever since cursorMarks were introduced in SOLR-5463 in 2014, cursorMark and
> timeAllowed parameters were not allowed in combination ("Can not search using
> both cursorMark and timeAllowed")
> , from [QueryComponent.java|#L359]]:
>
> {code:java}
>
> if (null != rb.getCursorMark() && 0 < timeAllowed) {
> // fundamentally incompatible
> throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Can not
> search using both " + CursorMarkParams.CURSOR_MARK_PARAM + " and " +
> CommonParams.TIME_ALLOWED);
> } {code}
> While theoretically impure to use them in combination, it is often desirable
> to support cursormarks-style deep paging and attempt to protect Solr nodes
> from runaway queries using timeAllowed, in the hopes that most of the time,
> the query completes in the allotted time, and there is no conflict.
>
> However if the query takes too long, it may be preferable to end the query
> and protect the Solr node and provide the user with a somewhat inaccurate
> sorted list. As noted in SOLR-6930, SOLR-5986 and others, timeAllowed is
> frequently used to prevent runaway load. In fact, cursorMark and
> shards.tolerant are allowed in combination, so any argument in favor of
> purity would be a bit muddied in my opinion.
>
> This was discussed once in the mailing list that I can find:
> [https://mail-archives.apache.org/mod_mbox/lucene-solr-user/201506.mbox/%3c5591740b.4080...@elyograg.org%3E]
> It did not look like there was strong support for preventing the combination.
>
> I have tested cursorMark and timeAllowed combination together, and even when
> partial results are returned because the timeAllowed is exceeded, the
> cursorMark response value is still valid and reasonable.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
