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

Propagate root subobjects setting to downsample indexes #115358

Merged
merged 6 commits into from
Oct 24, 2024

Conversation

kkrik-es
Copy link
Contributor

Setting this in downsample tests allows running them with both trial and basic license.

@kkrik-es kkrik-es added >non-issue :StorageEngine/Downsampling Downsampling (replacement for rollups) - Turn fine-grained time-based data into coarser-grained data Team:StorageEngine labels Oct 22, 2024
@kkrik-es kkrik-es self-assigned this Oct 22, 2024
@kkrik-es kkrik-es marked this pull request as ready for review October 24, 2024 07:46
@kkrik-es kkrik-es requested a review from a team as a code owner October 24, 2024 07:46
@elasticsearchmachine
Copy link
Collaborator

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

private static void addRootSubobjects(Map<String, Object> sourceIndexMappings, XContentBuilder builder) throws IOException {
if (sourceIndexMappings.containsKey("subobjects")) {
builder.field("subobjects", sourceIndexMappings.get("subobjects"));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm on the fence for this one, it should be safe but we're updating production logic to make tests happy.. I can add tests if we like it, or guard behind a flag to use in tests only.

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks ok...anyway if you look a bit below we use a MappingVisitor object to visit mappings...maybe that is a better way to go through mappings and apply changes. Indeed I wonder if we need to do this for nested subobjects properties too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, though I was thinking of going to the other direction and skipping objects entirely. Today, we don't propagate any object properties so subobjects:false should always be applicable..

Copy link
Member

Choose a reason for hiding this comment

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

or guard behind a flag to use in tests only.

I think that this should be behind a setting and use it only for tests now.

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.

I left one comment, LGTM otherwise.

private static void addRootSubobjects(Map<String, Object> sourceIndexMappings, XContentBuilder builder) throws IOException {
if (sourceIndexMappings.containsKey("subobjects")) {
builder.field("subobjects", sourceIndexMappings.get("subobjects"));
}
Copy link
Member

Choose a reason for hiding this comment

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

or guard behind a flag to use in tests only.

I think that this should be behind a setting and use it only for tests now.

@kkrik-es
Copy link
Contributor Author

Turns out the downsampling changes are not needed. I reverted them and both tests pass.

@kkrik-es kkrik-es added auto-backport Automatically create backport pull requests when merged v8.17.0 labels Oct 24, 2024
@kkrik-es kkrik-es merged commit 5c1a3ad into elastic:main Oct 24, 2024
16 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

The backport operation could not be completed due to the following error:

An unexpected error occurred when attempting to backport this PR.

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 115358

@kkrik-es
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x

Questions ?

Please refer to the Backport tool documentation

kkrik-es added a commit to kkrik-es/elasticsearch that referenced this pull request Oct 24, 2024
* Propagate root subobjects setting to downsample indexes

* exclude tests from rest compat

* remove subobjects propagation

(cherry picked from commit 5c1a3ad)
georgewallace pushed a commit to georgewallace/elasticsearch that referenced this pull request Oct 25, 2024
* Propagate root subobjects setting to downsample indexes

* exclude tests from rest compat

* remove subobjects propagation
kkrik-es added a commit that referenced this pull request Oct 25, 2024
… (#115577)

* Propagate root subobjects setting to downsample indexes (#115358)

* Propagate root subobjects setting to downsample indexes

* exclude tests from rest compat

* remove subobjects propagation

(cherry picked from commit 5c1a3ad)

* Update build.gradle
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Nov 4, 2024
* Propagate root subobjects setting to downsample indexes

* exclude tests from rest compat

* remove subobjects propagation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Automatically create backport pull requests when merged backport pending >non-issue :StorageEngine/Downsampling Downsampling (replacement for rollups) - Turn fine-grained time-based data into coarser-grained data Team:StorageEngine v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants