cpoerschke commented on code in PR #950:
URL: https://github.com/apache/solr/pull/950#discussion_r925734354
##########
solr/solrj/src/test/org/apache/solr/client/solrj/io/stream/StreamDecoratorTest.java:
##########
@@ -3030,29 +3030,13 @@ public void testPriorityStream() throws Exception {
assertEquals(tuples.size(), 4);
assertOrder(tuples, 5, 6, 7, 8);
- expression =
- StreamExpressionParser.parse(
- "priority(topic(collection1, collection1, q=\"a_s:hello\",
fl=\"id,a_i\", id=1000000, initialCheckpoint=0),"
- + "topic(collection1, collection1, q=\"a_s:hello1\",
fl=\"id,a_i\", id=2000000, initialCheckpoint=0))");
- stream = factory.constructStream(expression);
- context = new StreamContext();
- context.setSolrClientCache(cache);
- stream.setStreamContext(context);
tuples = getTuples(stream);
Collections.sort(tuples, comp);
// The Tuples from the second topic (Low priority) should be returned.
assertEquals(tuples.size(), 6);
assertOrder(tuples, 0, 1, 2, 3, 4, 9);
Review Comment:
The intention here is to get the lower priority tuples i.e. the next 6 after
the previous 4. However, `initialCheckpoint=0` as per
https://solr.apache.org/guide/solr/latest/query-guide/stream-source-reference.html#topic-parameters
actually means _"process all records that match query in the index"_ and 10
match overall.
It seems this test currently passes because as per SOLR-16286 reported by
@danrosher (see also #935) the `initialCheckpoints` parameter not honoured i.e.
the closing at line 3025 happens and it persists the checkpoints but then the
next expression simply uses the persisted checkpoints irrespective of the
`initialCheckpoint=0` that is passed in that expression.
It seems that continuing with the existing expression/stream better reflects
the intention of the test. See also
https://solr.apache.org/guide/solr/latest/query-guide/stream-decorator-reference.html#priority
which says _"The `priority` function will only emit a batch of tasks from one
of the queues each time it is called."_ i.e. with two queues there being two
`getTuples` calls seems logical?
@joel-bernstein @danrosher - what do 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: [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]
