Skip to content

Conversation

@kkrik-es
Copy link
Contributor

The reported test failure indicates that there are two identical persistent tasks proceeding, which is not an issue with the downsample action (this shouldn't be happening).

Fixes #122158

@kkrik-es kkrik-es added >test Issues or PRs that are addressing/adding tests :StorageEngine/Downsampling Downsampling (replacement for rollups) - Turn fine-grained time-based data into coarser-grained data Team:StorageEngine labels Feb 11, 2025
@kkrik-es kkrik-es self-assigned this Feb 11, 2025
@kkrik-es kkrik-es requested a review from martijnvg February 11, 2025 14:53
@kkrik-es kkrik-es marked this pull request as ready for review February 11, 2025 14:53
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

LGTM

try {
downsample(sourceIndex, targetIndex, config);
} catch (ResourceAlreadyExistsException e) {
} catch (ElasticsearchException e) {
Copy link
Member

Choose a reason for hiding this comment

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

A persistent task with the same id/name should result in a ResourceAlreadyExistsException. However maybe it is wrapped in another exception and that is why the test failed. I think just catching a parent exception is ok here. We could also maybe unwrap the cause? (ExceptionsHelper)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The log shows that actually both tasks ran, somehow.. The exception is from the second task failing to write to the target index, as it's marked as read-only by the first one after populating it. I'm not sure how it is possible to have both tasks to run, couldn't reproduce it.

@kkrik-es kkrik-es enabled auto-merge (squash) February 12, 2025 10:03
@kkrik-es kkrik-es disabled auto-merge February 12, 2025 13:07
@kkrik-es kkrik-es merged commit 65f6b44 into elastic:main Feb 12, 2025
2 of 4 checks passed
@kkrik-es kkrik-es deleted the fix/122158 branch March 11, 2025 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:StorageEngine/Downsampling Downsampling (replacement for rollups) - Turn fine-grained time-based data into coarser-grained data Team:StorageEngine >test Issues or PRs that are addressing/adding tests v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] DownsampleActionSingleNodeTests testDuplicateDownsampleRequest failing

3 participants