Skip to content

Commit

Permalink
[Session management] update cleanup query to allow partial search res…
Browse files Browse the repository at this point in the history
…ults for PIT query (elastic#203413)

Closes elastic#203440

### Summary
Update session cleanup task by adding the partial search results flag to
the PIT query as well and not just the search query.

#### Notes 
In the previous “fix”, the partial search results flag was incorrectly
added to the search query that depended on the PIT query. However, the
correct way is to set the flag when we openPointInTimeQuery which is
then used in the subsequent search query

### Release notes
Fixes error with opening point in time query for session deletion by now
accounting for partial results.


Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.


- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] The PR description includes the appropriate Release Notes section,
and the correct `release_node:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
  • Loading branch information
SiddharthMantri authored Dec 10, 2024
1 parent 2fa8f47 commit 91fec7a
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ describe('Session index', () => {
{
index: aliasName,
keep_alive: '5m',
allow_partial_search_results: true,
},
{ ignore: [404], meta: true }
);
Expand All @@ -454,6 +455,7 @@ describe('Session index', () => {
{
index: aliasName,
keep_alive: '5m',
allow_partial_search_results: true,
},
{ meta: true }
);
Expand All @@ -473,7 +475,6 @@ describe('Session index', () => {
expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1);
expect(mockElasticsearchClient.search).toHaveBeenCalledWith({
_source_includes: 'usernameHash,provider',
allow_partial_search_results: true,
sort: '_shard_doc',
track_total_hits: false,
search_after: undefined,
Expand Down Expand Up @@ -556,7 +557,6 @@ describe('Session index', () => {
expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1);
expect(mockElasticsearchClient.search).toHaveBeenCalledWith({
_source_includes: 'usernameHash,provider',
allow_partial_search_results: true,
sort: '_shard_doc',
track_total_hits: false,
search_after: undefined,
Expand Down Expand Up @@ -651,7 +651,6 @@ describe('Session index', () => {
expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1);
expect(mockElasticsearchClient.search).toHaveBeenCalledWith({
_source_includes: 'usernameHash,provider',
allow_partial_search_results: true,
sort: '_shard_doc',
track_total_hits: false,
search_after: undefined,
Expand Down Expand Up @@ -740,7 +739,6 @@ describe('Session index', () => {
expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1);
expect(mockElasticsearchClient.search).toHaveBeenCalledWith({
_source_includes: 'usernameHash,provider',
allow_partial_search_results: true,
sort: '_shard_doc',
track_total_hits: false,
search_after: undefined,
Expand Down Expand Up @@ -854,7 +852,6 @@ describe('Session index', () => {
expect(mockElasticsearchClient.search).toHaveBeenCalledTimes(1);
expect(mockElasticsearchClient.search).toHaveBeenCalledWith({
_source_includes: 'usernameHash,provider',
allow_partial_search_results: true,
sort: '_shard_doc',
track_total_hits: false,
search_after: undefined,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,10 @@ export class SessionIndex {
{
index: this.aliasName,
keep_alive: SESSION_INDEX_CLEANUP_KEEP_ALIVE,
// @ts-expect-error client support this option, but it is not documented and typed yet.
// once support added we should remove this expected type error
// https://github.com/elastic/elasticsearch-specification/issues/3144
allow_partial_search_results: true,
},
{ ignore: [404], meta: true }
);
Expand All @@ -841,6 +845,10 @@ export class SessionIndex {
{
index: this.aliasName,
keep_alive: SESSION_INDEX_CLEANUP_KEEP_ALIVE,
// @ts-expect-error client support this option, but it is not documented and typed yet.
// once support added we should remove this expected type error
// https://github.com/elastic/elasticsearch-specification/issues/3144
allow_partial_search_results: true,
},
{ meta: true }
));
Expand All @@ -857,7 +865,6 @@ export class SessionIndex {
size: SESSION_INDEX_CLEANUP_BATCH_SIZE,
sort: '_shard_doc',
track_total_hits: false, // for performance
allow_partial_search_results: true,
});
const { hits } = searchResponse.hits;
if (hits.length > 0) {
Expand Down

0 comments on commit 91fec7a

Please sign in to comment.