Skip to content

add basic warming query configs #844

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 11, 2025
Merged

add basic warming query configs #844

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c342c56
add basic warming query configs
waziqi89 Apr 8, 2025
7bf1877
add test
waziqi89 Apr 9, 2025
81bada3
resolve comments
waziqi89 Apr 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/main/java/com/yelp/nrtsearch/server/warming/WarmingUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,8 @@
import com.yelp.nrtsearch.server.grpc.NestedQuery;
import com.yelp.nrtsearch.server.grpc.Query;
import com.yelp.nrtsearch.server.grpc.SearchRequest;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class WarmingUtils {
private static final Logger logger = LoggerFactory.getLogger(WarmingUtils.class);

public static SearchRequest simplifySearchRequestForWarming(
SearchRequest.Builder searchRequestBuilder) {
Expand Down
61 changes: 35 additions & 26 deletions src/test/java/com/yelp/nrtsearch/server/warming/WarmerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public void setup() throws IOException {
NrtsearchConfig config = new NrtsearchConfig(new ByteArrayInputStream(configStr.getBytes()));
s3 = s3Provider.getAmazonS3();
remoteBackend = new S3Backend(config, s3);
warmer = new Warmer(remoteBackend, service, index, 2);
warmer = new Warmer(remoteBackend, service, index, 4);
}

@Test
Expand Down Expand Up @@ -110,7 +110,10 @@ public void testWarmFromS3()
@Test
public void testWarmFromS3_basic()
throws IOException, SearchHandler.SearchHandlerException, InterruptedException {
Warmer warmerWithBasic = new Warmer(remoteBackend, service, index, 2, 30);
Warmer warmerWithBasic = new Warmer(remoteBackend, service, index, 4, 80);
// nextInt(100) with seed 98795 will always return 15, 76, 87, 97; If we set the Perc at 80,
// first two queries will be stripped while the last two will be kept
warmerWithBasic.randomThreadLocal.get().setSeed(98795);

List<String> testSearchRequestsJson = getTestSearchRequestsAsJsonStrings();
byte[] warmingBytes = getWarmingBytes(testSearchRequestsJson);
Expand All @@ -119,8 +122,6 @@ public void testWarmFromS3_basic()
IndexState mockIndexState = mock(IndexState.class);
SearchHandler mockSearchHandler = mock(SearchHandler.class);

// nextInt(100) for this seed is: 28, 33, 20, 10
warmerWithBasic.randomThreadLocal.get().setSeed(1234);
warmerWithBasic.warmFromS3(mockIndexState, 0, mockSearchHandler);

for (SearchRequest testRequest : getTestBasicSearchRequests()) {
Expand Down Expand Up @@ -185,7 +186,7 @@ private byte[] getWarmingBytes(List<String> queryStrings) throws IOException {

private List<SearchRequest> getTestSearchRequests() {
List<SearchRequest> testRequests = new ArrayList<>();
for (int i = 0; i < 2; i++) {
for (int i = 0; i < 4; i++) {
SearchRequest searchRequest =
SearchRequest.newBuilder()
.setIndexName(index)
Expand All @@ -205,33 +206,41 @@ private List<SearchRequest> getTestSearchRequests() {

private List<SearchRequest> getTestBasicSearchRequests() {
List<SearchRequest> testRequests = new ArrayList<>();
SearchRequest searchRequest =
SearchRequest.newBuilder()
.setIndexName(index)
.setQuery(Query.newBuilder().setTermQuery(TermQuery.newBuilder().setField("field0")))
.build();
testRequests.add(searchRequest);

searchRequest =
SearchRequest.newBuilder()
.setIndexName(index)
.setQuery(
Query.newBuilder()
.setFunctionScoreQuery(
FunctionScoreQuery.newBuilder()
.setQuery(
Query.newBuilder()
.setTermQuery(TermQuery.newBuilder().setField("field1")))
.setScript(Script.newBuilder().setLang("js").setSource("3 * 5"))))
.build();
testRequests.add(searchRequest);
int i = 0;
for (; i < 2; i++) {
SearchRequest searchRequest =
SearchRequest.newBuilder()
.setIndexName(index)
.setQuery(
Query.newBuilder().setTermQuery(TermQuery.newBuilder().setField("field" + i)))
.build();
testRequests.add(searchRequest);
}

for (; i < 4; i++) {
Copy link
Member

@fragosoluana fragosoluana Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

SearchRequest searchRequest =
SearchRequest.newBuilder()
.setIndexName(index)
.setQuery(
Query.newBuilder()
.setFunctionScoreQuery(
FunctionScoreQuery.newBuilder()
.setQuery(
Query.newBuilder()
.setTermQuery(TermQuery.newBuilder().setField("field" + i)))
.setScript(Script.newBuilder().setLang("js").setSource("3 * 5"))))
.build();
testRequests.add(searchRequest);
}

return testRequests;
}

private List<String> getTestSearchRequestsAsJsonStrings() {
return List.of(
"{\"indexName\":\"test_index\",\"query\":{\"functionScoreQuery\":{\"query\":{\"termQuery\":{\"field\":\"field0\"}},\"script\":{\"lang\":\"js\",\"source\":\"3 * 5\"}}}}",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"{\"indexName\":\"test_index\",\"query\":{\"functionScoreQuery\":{\"query\":{\"termQuery\":{\"field\":\"field1\"}},\"script\":{\"lang\":\"js\",\"source\":\"3 * 5\"}}}}");
"{\"indexName\":\"test_index\",\"query\":{\"functionScoreQuery\":{\"query\":{\"termQuery\":{\"field\":\"field1\"}},\"script\":{\"lang\":\"js\",\"source\":\"3 * 5\"}}}}",
"{\"indexName\":\"test_index\",\"query\":{\"functionScoreQuery\":{\"query\":{\"termQuery\":{\"field\":\"field2\"}},\"script\":{\"lang\":\"js\",\"source\":\"3 * 5\"}}}}",
"{\"indexName\":\"test_index\",\"query\":{\"functionScoreQuery\":{\"query\":{\"termQuery\":{\"field\":\"field3\"}},\"script\":{\"lang\":\"js\",\"source\":\"3 * 5\"}}}}");
}
}
Loading