fix: Prevent NPE in query_lookup when hit index differs from configur… - #7098
Conversation
…ed index ExistingDocumentQueryManager.dropAndReleaseFoundEvents looked up pending bulk operations using hit.index(), the index reported on each search hit. When the OpenSearch sink writes to an alias or datastream, the hit reports the concrete backing index (e.g. my-alias-000001), which differs from the configured index/alias key (my-alias) used to store the pending operations. The lookup returned null and threw a NullPointerException on every query cycle, wedging the query loop: the affected documents were never indexed, dropped, or sent to the DLQ, and only a full pipeline restart recovered (losing buffered events). Correlate each msearch response to the index key of the request that produced it. buildMultiSearchRequest now records the index key per search request in order, and dropAndReleaseFoundEvents resolves the pending operations from that ordered key list (msearch preserves request order) instead of hit.index(). A null-guard ensures an unexpected response can never NPE and block the loop. Adds a regression test asserting that a found duplicate is dropped and released when the hit's index differs from the configured index. Verified the test fails (NPE) against the previous implementation. Resolves opensearch-project#6902 Signed-off-by: Nagendra Mohan <nagendramohan1990@gmail.com>
PR Reviewer Guide 🔍(Review updated until commit 632ea79)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 632ea79
Previous suggestionsSuggestions up to commit a278c27
|
Address review feedback on the request/response correlation. Previously orderedIndexKeys was populated inside the MsearchRequest.of(...) builder lambda, so alignment with the response order relied on the lambda running synchronously and in order. Precompute the (index key, term-value chunk) pairs into a deterministic list first, then build both orderedIndexKeys and the msearch searches from that same list in the same order, so the alignment is guaranteed by construction. No behavior change. Signed-off-by: Nagendra Mohan <nagendramohan1990@gmail.com>
|
Persistent review updated to latest commit 632ea79 |
|
Gentle bump on this one 🙂 — it fixes an NPE in ExistingDocumentQueryManager.dropAndReleaseFoundEvents where, for alias/datastream sinks, hit.index() (the concrete backing index) never matches the configured alias key, so the lookup NPEs every query cycle and wedges the loop — affected documents are never indexed, dropped, or sent to the DLQ until a full restart. CI is green and there's a regression test that fails against the old code. @dlvenable would appreciate your review when you have a cycle. Thanks! |
1 participant
…ed index
ExistingDocumentQueryManager.dropAndReleaseFoundEvents looked up pending bulk operations using hit.index(), the index reported on each search hit. When the OpenSearch sink writes to an alias or datastream, the hit reports the concrete backing index (e.g. my-alias-000001), which differs from the configured index/alias key (my-alias) used to store the pending operations. The lookup returned null and threw a NullPointerException on every query cycle, wedging the query loop: the affected documents were never indexed, dropped, or sent to the DLQ, and only a full pipeline restart recovered (losing buffered events).
Correlate each msearch response to the index key of the request that produced it. buildMultiSearchRequest now records the index key per search request in order, and dropAndReleaseFoundEvents resolves the pending operations from that ordered key list (msearch preserves request order) instead of hit.index(). A null-guard ensures an unexpected response can never NPE and block the loop.
Adds a regression test asserting that a found duplicate is dropped and released when the hit's index differs from the configured index. Verified the test fails (NPE) against the previous implementation.
Resolves #6902
Description
ExistingDocumentQueryManager.dropAndReleaseFoundEvents looked up pending bulk operations by hit.index(). When the OpenSearch sink writes to an alias or datastream, each search hit reports the concrete backing index (e.g. my-alias-000001), which differs from the configured index/alias key (my-alias) used to store the pending operations. bulkOperationsWaitingForQuery.get(hit.index()) returned null and threw a NullPointerException on every query cycle, wedging the query loop — affected documents were never indexed, dropped, or sent to the DLQ, and only a full pipeline restart recovered (losing buffered events).
Fix
Correlate each msearch response to the index key of the request that produced it. buildMultiSearchRequest now records the index key per search request in order, and dropAndReleaseFoundEvents resolves the pending operations from that ordered key list (msearch preserves request order) instead of hit.index(). A null-guard ensures an unexpected response can never NPE and block the loop.
Issues Resolved
Resolves #6902
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.