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

Downsampling: copyindex.hidden setting from source #89177

Merged
merged 6 commits into from
Aug 17, 2022

Conversation

csoulios
Copy link
Contributor

@csoulios csoulios commented Aug 8, 2022

Rollup indices are initially created as hidden (index.hidden: true).

At the end of the rollup process, we must set this setting to the value the source index has

@csoulios csoulios added >non-issue :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data v8.5.0 labels Aug 8, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Aug 8, 2022
@elasticsearchmachine
Copy link
Collaborator

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

Comment on lines 271 to 273
if (sourceIndexMetadata.isHidden() == false) {
settings.put(IndexMetadata.SETTING_INDEX_HIDDEN, false);
}
Copy link
Contributor Author

@csoulios csoulios Aug 8, 2022

Choose a reason for hiding this comment

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

There's a small catch here. If the setting index.hidden was not set at all in the source index, the default value is false. Here, we explicitly set the value to false. Hopefully, this does not interfere with anything else.

Copy link
Member

Choose a reason for hiding this comment

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

Could we do something like

if (sourceIndexMetadata.isHidden() == false && sourceIndexMetadata.getSettings().keySet().contains(IndexMetadata.SETTING_INDEX_HIDDEN)) {
    settings.put(IndexMetadata.SETTING_INDEX_HIDDEN, false);
}

To only set the setting if it does indeed exist on the source as explicitly set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the rollup index is created setting index.hidden is set to true. So, after rollup ends we want to revert this value.

I changed the code so that it removes setting (calling setNull()) if it is not set at all in the source index.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me 👍

@salvatore-campagna
Copy link
Contributor

Just a minor comment. For the rest LGTM.

Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, I left one suggestion though, about how we might be able to do it without the explicit setting.

Comment on lines 271 to 273
if (sourceIndexMetadata.isHidden() == false) {
settings.put(IndexMetadata.SETTING_INDEX_HIDDEN, false);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we do something like

if (sourceIndexMetadata.isHidden() == false && sourceIndexMetadata.getSettings().keySet().contains(IndexMetadata.SETTING_INDEX_HIDDEN)) {
    settings.put(IndexMetadata.SETTING_INDEX_HIDDEN, false);
}

To only set the setting if it does indeed exist on the source as explicitly set?

Copy link
Contributor

@salvatore-campagna salvatore-campagna left a comment

Choose a reason for hiding this comment

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

LGTM

@csoulios csoulios merged commit 03f3c81 into elastic:main Aug 17, 2022
@csoulios csoulios deleted the rollup-hidden-fix branch August 17, 2022 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants