Skip to content

Add index.mode: time_series #77626

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

Merged
merged 13 commits into from
Sep 14, 2021
Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Sep 13, 2021

This adds a setting to enable time series mode that is hidden by the
es.index_mode_feature_flag_registered feature flag. All it does right
now is make sure you haven't configured index sorting and partitioning.
This gives us a place to "hang" all of our further work on time series
mode.

Time series mode will entirely take over index sorting. We don't believe
you'll ever be able to configure sorting yourself when you are in time
series mode. We don't expect time series mode to support index
partitioning when we first build it but we'd like to get there.

@nik9000 nik9000 requested review from imotov and csoulios September 13, 2021 13:45
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 13, 2021
@elasticmachine
Copy link
Collaborator

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

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2021

This is a tiny tiny tiny part of the rebuild of #75638. But opening it is a good sign! I feel like I really can get to the meat of the rebuild soon.

This adds a setting to enable time series mode that is hidden by the
`es.index_mode_feature_flag_registered` feature flag. All it does right
now is make sure you haven't configured index sorting and partitioning.
This gives us a place to "hang" all of our further work on time series
mode.

Time series mode will entirely take over index sorting. We don't believe
you'll ever be able to configure sorting yourself when you are in time
series mode. We don't expect time series mode to support index
partitioning when we first build it but we'd like to get there.
Copy link
Contributor

@mark-vieira mark-vieira 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 assume you ran a test build locally with -Dbuild.snapshot=false to see the tests were ok? We don't actually run in that configuration in PR or intake jobs.

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2021

LGTM. I assume you ran a test build locally with -Dbuild.snapshot=false to see the tests were ok? We don't actually run in that configuration in PR or intake jobs.

I hadn't. Let me give that a go.

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2021

I hadn't. Let me give that a go.

Well first time trying it everything crashed. I've stared with --max-workers 2 to see how that goes.

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2021

Well first time trying it everything crashed. I've stared with --max-workers 2 to see how that goes.

Yeah, the load average was 130 and the machine has 12 cores....

With -max-workers 2 I'm getting load average of 10.

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2021

After a few false starts I have some progress!

<====---------> 34% EXECUTING [50m 57s]

It's been a long, long, long time since I ran the entire suite locally.

return IndexMode.VALIDATE_WITH_SETTINGS.iterator();
}
},
Property.IndexScope
Copy link
Member Author

Choose a reason for hiding this comment

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

This should probably also have Property.Final.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed Property.Final seems like that way to go here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should INDEX_ROUTING_PARTITION_SIZE_SETTING be Final as well? It is not marked Final but it seems like it shouldn't be set after index creation.

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #77669 to talk about that.

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

LGTM.

return IndexMode.VALIDATE_WITH_SETTINGS.iterator();
}
},
Property.IndexScope
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed Property.Final seems like that way to go here.

return IndexMode.VALIDATE_WITH_SETTINGS.iterator();
}
},
Property.IndexScope
Copy link
Contributor

Choose a reason for hiding this comment

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

Should INDEX_ROUTING_PARTITION_SIZE_SETTING be Final as well? It is not marked Final but it seems like it shouldn't be set after index creation.

@nik9000
Copy link
Member Author

nik9000 commented Sep 14, 2021

Lovely:

$  ./gradlew --max-workers 2 -Dbuild.snapshot=false -Dlicense.key="x-pack/license-tools/src/test/resources/public.key" -p qa check
...

BUILD FAILED in 1h 46m 30s

Failed overnight. :qa:snapshot-based-recoveries:gcs:integTest got an empty reply from the server. Checking it again locally.

I think, maybe, we need a way to make CI do this for me. Or we need some better way for me to do it locally.

@nik9000
Copy link
Member Author

nik9000 commented Sep 14, 2021

Failed overnight. :qa:snapshot-based-recoveries:gcs:integTest got an empty reply from the server. Checking it again locally.

Didn't fail locally on second run, either with build.snapshot=false or without it. Gremlins killed my build.

@nik9000 nik9000 merged commit 962976b into elastic:master Sep 14, 2021
@nik9000
Copy link
Member Author

nik9000 commented Sep 14, 2021

I've tested this a bunch locally and it looks ok. I've merged to master and manually kicked off a non-snapshot build using it. I'm going to wait to backport until it passes.

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Sep 15, 2021
This adds a setting to enable time series mode that is hidden by the
`es.index_mode_feature_flag_registered` feature flag. All it does right
now is make sure you haven't configured index sorting and partitioning.
This gives us a place to "hang" all of our further work on time series
mode.

Time series mode will entirely take over index sorting. We don't believe
you'll ever be able to configure sorting yourself when you are in time
series mode. We don't expect time series mode to support index
partitioning when we first build it but we'd like to get there.
nik9000 added a commit that referenced this pull request Sep 15, 2021
This adds a setting to enable time series mode that is hidden by the
`es.index_mode_feature_flag_registered` feature flag. All it does right
now is make sure you haven't configured index sorting and partitioning.
This gives us a place to "hang" all of our further work on time series
mode.

Time series mode will entirely take over index sorting. We don't believe
you'll ever be able to configure sorting yourself when you are in time
series mode. We don't expect time series mode to support index
partitioning when we first build it but we'd like to get there.
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Sep 20, 2021
Now that elastic#77626 has landed we can test tsdb *things* against 7.16.
@nik9000 nik9000 removed the v7.16.0 label Oct 1, 2021
@nik9000
Copy link
Member Author

nik9000 commented Oct 1, 2021

I'm pulling this back out of 7.16 because we don't really have a chance of making a useful tsdb in time. So there isn't really any need to have the setting in that version.

@nik9000
Copy link
Member Author

nik9000 commented Oct 1, 2021

#78570 is the revert PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v8.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants