add basic warming query configs #844
Conversation
There was a problem hiding this comment.
I noticed that WarmerTest changes from v0.x weren't added in this PR, just double checking if this is intentional
src/main/java/com/yelp/nrtsearch/server/warming/WarmingUtils.java
Outdated
Show resolved
Hide resolved
Forget to add it, let me fix this. |
src/test/java/com/yelp/nrtsearch/server/warming/WarmerTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/yelp/nrtsearch/server/warming/WarmerTest.java
Outdated
Show resolved
Hide resolved
| private List<String> getTestSearchRequestsAsJsonStrings() { | ||
| return List.of( | ||
| "{\"indexName\":\"test_index\",\"query\":{\"termQuery\":{\"field\":\"field0\"}}}", | ||
| "{\"indexName\":\"test_index\",\"query\":{\"termQuery\":{\"field\":\"field1\"}}}"); | ||
| "{\"indexName\":\"test_index\",\"query\":{\"functionScoreQuery\":{\"query\":{\"termQuery\":{\"field\":\"field0\"}},\"script\":{\"lang\":\"js\",\"source\":\"3 * 5\"}}}}", |
There was a problem hiding this comment.
Before we were testing the same query twice to test it being simplified or not. With the last commit change, we have 2 extra identical queries and I am just wondering what is being tested with these extra identical queries. Just wondering if we need them or if we can keep only two and simplify the test code by removing the extra for loops in getTestBasicSearchRequests.
There was a problem hiding this comment.
Adding 2 more lines to ensure the "random" works exactly as we expected. Only 2 queries might be coincident.
We ain't aiming to test the major stripping logic here
| testRequests.add(searchRequest); | ||
| } | ||
|
|
||
| for (; i < 4; i++) { |
There was a problem hiding this comment.
minor suggestion: use for (int i = 2; i < 4; i++) to be more clear that this for loop will add 2 requests. When I first read it quickly, I thought it was adding 4.
port the v0 stripped warming query code into v1.