Skip to content
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

Jump to bottom

Set option ignore_unavailable for querying #18146

Merged
merged 11 commits into from
Feb 8, 2024
Merged

Set option ignore_unavailable for querying #18146

merged 11 commits into from
Feb 8, 2024

Conversation

patrickmann
Copy link
Contributor

@patrickmann patrickmann commented Feb 2, 2024 •
edited
Loading

Resolves #18127

Description

When issuing a simple search query we now set option ignoreUnavailable=true and allowNoIndices=true. This serves to suppress the exception in case of missing indices, but otherwise leaves behavior unaltered.

In the case of scroll requests we also catch and handle the exception raised by empty results.

Motivation and Context

Avoid exceptions when there is a race between querying and deleting an index. See linked issue for details.

@patrickmann patrickmann requested a review from todvora February 5, 2024 17:25
todvora
todvora reviewed Feb 6, 2024
View reviewed changes
Copy link
Contributor

@todvora todvora left a comment

Choose a reason for hiding this comment

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

Just two details - the added catch without propagation and the allowNoIndices option introduced together with the ignoreUnavailable.

...ge-elasticsearch7/src/main/java/org/graylog/storage/elasticsearch7/MoreSearchAdapterES7.java Outdated
try {
chunkedResult = multiChunkResultRetriever.retrieveChunkedResult(chunkCommand);
} catch (IllegalArgumentException e) {
LOG.warn("Ignoring invalid scroll request", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain why we catch the exception here and don't propagate it further?

Copy link
Contributor Author

@patrickmann patrickmann Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Added comment in code.
This allows aggregation or correlation event queries to complete, in the presence of unavailable indices. Ugly, but I don't see where we could add the necessary indicesOptions in an aggregation query.

Copy link
Contributor

@luk-kaminski luk-kaminski Feb 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@patrickmann - I'm sorry if I am saying stupid things, but I do not understand this one.
MultiChunkResultRetriever has 4 possible implementations, and you have added options to all 4 of them.
Why is it still throwing exceptions on unavailable indices and requires to handle those surprising exceptions manually?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that scroll search, when executed on unavailable indices, would not return response with _scroll_id field.
Because of that construction will fail on this line:

graylog2-server/graylog-storage-opensearch2/src/main/java/org/graylog/storage/opensearch2/ScrollResultOS2.java

Line 48 in 8132032

checkArgument(initialResult.getScrollId() != null, "Unable to extract scroll id from supplied search result!");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@luk-kaminski
You are right - when scroll searching, the exception is thrown from the constructor you mention; not because of a missing ignoreUnavailable option.
Is there a more elegant way of handling that? I was hesitant to make changes to scroll result processing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've committed and pushed a proposal of another method of handling that. Will be grateful for feedback.

...sticsearch7/src/main/java/org/graylog/storage/elasticsearch7/views/ElasticsearchBackend.java Outdated
@@ -255,7 +254,7 @@ public QueryResult doRun(SearchJob job, Query query, ESGeneratedQueryContext que
return new SearchRequest()
.source(searchTypeQueries.get(searchTypeId))
.indices(indices.toArray(new String[0]))
.indicesOptions(IndicesOptions.fromOptions(false, false, true, false));
.indicesOptions(IndicesOptions.fromOptions(true, true, true, false));
Copy link
Contributor

Choose a reason for hiding this comment

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

allowNoIndices is now set to true. Is this by purpose? Does it help with the issue which is solved by this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case a query contains only invalid indices, we need allowNoIndices for it to still succeed. This turned up in unit testing.

@patrickmann patrickmann requested a review from todvora February 6, 2024 08:49
@patrickmann patrickmann marked this pull request as ready for review February 6, 2024 11:10
@todvora todvora requested a review from luk-kaminski February 6, 2024 13:23
todvora
todvora approved these changes Feb 8, 2024
View reviewed changes
Copy link
Contributor

@todvora todvora left a comment

Choose a reason for hiding this comment

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

LGTM!

@todvora todvora merged commit 7acdc1d into master Feb 8, 2024
5 checks passed
@todvora todvora deleted the queryUnavailable branch February 8, 2024 10:46
patrickmann added a commit that referenced this pull request Feb 8, 2024
@patrickmann
Set option ignore_unavailable for querying (#18146)
d83f9e4 
* set option ignore_unavailable

* CL

* suppress missing index exception in event query

* unit tests

* ignore failing scroll request

* more strict index options

* add comments

* use symbolic constants

* No exception when search request returns no scrollID value and there are no available indices

* annotate nullable

---------

Co-authored-by: luk-kaminski <[email protected]>

(cherry picked from commit 7acdc1d)
todvora pushed a commit that referenced this pull request Feb 9, 2024
@patrickmann
Set option ignore_unavailable for querying (#18146) (#18198)
18e29dd 
* Set option ignore_unavailable for querying (#18146)

* set option ignore_unavailable

* CL

* suppress missing index exception in event query

* unit tests

* ignore failing scroll request

* more strict index options

* add comments

* use symbolic constants

* No exception when search request returns no scrollID value and there are no available indices

* annotate nullable

---------

Co-authored-by: luk-kaminski <[email protected]>

(cherry picked from commit 7acdc1d)

* fix merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@todvora todvora todvora approved these changes

@luk-kaminski luk-kaminski Awaiting requested review from luk-kaminski

Assignees
No one assigned
Labels
backport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Race condition during index delete
3 participants
@patrickmann @todvora @luk-kaminski