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.

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

Fixes assertion check in RollupShardIndexer #68878

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Feb 11, 2021

This commit removes the assertion in RollupShardIndexer that verifies that
temporary files are deleted. Since it is the responsibility of the indexer
to instruct the OS to delete files, it may not do so in a timely manner. This
results in a potentially flaky assertion. Instead, a new unit test is introduced
that will introspect the indexer and assert that it had successfully called
for the files to be deleted.

Closes #68609.

This commit removes the assertion in RollupShardIndexer that verifies that
temporary files are deleted. Since it is the responsibility of the indexer
to instruct the OS to delete files, it may not do so in a timely manner. This
results in a potentially flaky assertion. Instead, a new unit test is introduced
that will introspect the indexer and assert that it had successfully called
for the files to be deleted.

Closes elastic#68609.
@talevy talevy added >test Issues or PRs that are addressing/adding tests :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) labels Feb 11, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (Team:Analytics)

@talevy talevy requested a review from csoulios February 11, 2021 01:23
Copy link
Contributor

@csoulios csoulios left a comment

Choose a reason for hiding this comment

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

LGTM!

@talevy talevy merged commit a0d8a89 into elastic:master Feb 11, 2021
@talevy talevy deleted the fixassertrollup branch February 11, 2021 23:07
talevy added a commit to talevy/elasticsearch that referenced this pull request Feb 16, 2021
This commit removes the assertion in RollupShardIndexer that verifies that
temporary files are deleted. Since it is the responsibility of the indexer
to instruct the OS to delete files, it may not do so in a timely manner. This
results in a potentially flaky assertion. Instead, a new unit test is introduced
that will introspect the indexer and assert that it had successfully called
for the files to be deleted.

Closes elastic#68609.
talevy added a commit that referenced this pull request Feb 16, 2021
This commit removes the assertion in RollupShardIndexer that verifies that
temporary files are deleted. Since it is the responsibility of the indexer
to instruct the OS to delete files, it may not do so in a timely manner. This
results in a potentially flaky assertion. Instead, a new unit test is introduced
that will introspect the indexer and assert that it had successfully called
for the files to be deleted.

backport of #68878.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v7.12.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] RollupActionIT testRollupIndexAndSetNewRollupPolicy failing
4 participants