Skip to content

NPE on batched query execution when the request is part of PIT with alias filters #128552

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.

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 19 commits into from
Jun 3, 2025

Conversation

drempapis
Copy link
Contributor

@drempapis drempapis commented May 28, 2025

As in the description of the issue (#128270)

When the conditions are met for the new feature Batched query execution (#121885) to be applied, the specific use case where executing a request within a point in time with alias filters leads to a NPE.

In SearchQueryThenFetchAsyncAction#isPartOfPIT, a null namedWriteableRegistry is passed when trying to access the search context id of the point in time builder. Since the alias filters use the registry when decoding the pit ID, this results in a NPE.

In this work, the namedWriteableRegistry is propagated to the isPartOfPIT method to facilitate context retrieval.

@drempapis drempapis added >bug Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch :Search Foundations/Search Catch all for Search Foundations labels May 28, 2025
@elasticsearchmachine
Copy link
Collaborator

Hi @drempapis, I've created a changelog YAML for you.

@drempapis drempapis changed the title add fix code NPE on batched query execution when the request is part of PIT with alias filters May 28, 2025
@drempapis drempapis marked this pull request as ready for review May 29, 2025 15:31
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search-foundations (Team:Search Foundations)

public void testIndexWithAlias() {
String indexName = "index_1";
String alias = "alias_1";
assertAcked(indicesAdmin().prepareCreate(indexName).setSettings(indexSettings(10, 0)).addAlias(new Alias(alias)));
Copy link
Member

Choose a reason for hiding this comment

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

Did this test fail without the fix? I was unclear on whether we need to add a filter to the alias, or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the filter is required. I inadvertently committed a version that did not include the filtering logic. Updating with some enhancements

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

I left a couple of comments on the test, the fix makes sense to me. LGTM, no need for another round once the comments are addressed.

@drempapis drempapis merged commit d77d2cf into elastic:8.19 Jun 3, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search Foundations/Search Catch all for Search Foundations Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch v8.19.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants