You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/07/20 14:59:24 UTC
[GitHub] [solr] cpoerschke opened a new pull request, #950: SOLR-16286: simplify StreamDecoratorTest.testPriorityStream
cpoerschke opened a new pull request, #950:
URL: https://github.com/apache/solr/pull/950
Noticed as part of https://issues.apache.org/jira/browse/SOLR-16286 -- please see code comments for details.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #950: SOLR-16286: simplify StreamDecoratorTest.testPriorityStream
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on pull request #950: SOLR-16286: simplify StreamDecoratorTest.test[Parallel]PriorityStream
Posted by GitBox <gi...@apache.org>.
cpoerschke commented on PR #950:
URL: https://github.com/apache/solr/pull/950#issuecomment-1208394501
Proceeding to merge this test change, happy to follow-up on any post-merge feedback.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke merged pull request #950: SOLR-16286: simplify StreamDecoratorTest.test[Parallel]PriorityStream
Posted by GitBox <gi...@apache.org>.
cpoerschke merged PR #950:
URL: https://github.com/apache/solr/pull/950
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] cpoerschke commented on a diff in pull request #950: SOLR-16286: simplify StreamDecoratorTest.testPriorityStream
Posted by GitBox <gi...@apache.org>.
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 persisted check points are 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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org