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

Test time series id in RecoverySourceHandlerTests #84996

Merged
merged 3 commits into from
Mar 17, 2022

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Mar 15, 2022

We were just testing the standard _id. This converts
RecoverySourceHandlerTests into a MapperServiceTestCase so it can do
a "real" parse of the document which'll be very important given our
future plans to make _id non-stored in tsdb.

@nik9000 nik9000 added >test Issues or PRs that are addressing/adding tests :StorageEngine/TSDB You know, for Metrics v8.2.0 labels Mar 15, 2022
@nik9000 nik9000 requested a review from henningandersen March 15, 2022 16:11
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Mar 15, 2022
@elasticmachine
Copy link
Collaborator

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

We were just testing the standard `_id`. This converts
`RecoverySourceHandlerTests` into a `MapperServiceTestCase` so it can do
a "real" parse of the document which'll be very important given our
future plans to make `_id` non-stored in tsdb.
@nik9000
Copy link
Member Author

nik9000 commented Mar 15, 2022

run elasticsearch-ci/part-2

Copy link
Contributor

@henningandersen henningandersen left a comment

Choose a reason for hiding this comment

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

LGTM.

Left a couple of comments that I would like to see fixed, otherwise good.

@nik9000 nik9000 added auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) and removed auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Mar 16, 2022
@nik9000 nik9000 merged commit cb92016 into elastic:master Mar 17, 2022
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Mar 17, 2022
This should be enough to detect if tsdb's `_id` field mapper changes
enough to cause trouble for `Engine`. I suspect that in the end we'll
need something more like the changes that elastic#84996 did for
RecoverySourceHandlerTests but that's a much bigger change that I'd
prefer to hold back until we need it. If tsdb's `_id` field mapper
changes enough to cause trouble for `Engine`.
nik9000 added a commit that referenced this pull request Mar 21, 2022
This should be enough to detect if tsdb's `_id` field mapper changes
enough to cause trouble for `Engine`. I suspect that in the end we'll
need something more like the changes that #84996 did for
RecoverySourceHandlerTests but that's a much bigger change that I'd
prefer to hold back until we need it. If tsdb's `_id` field mapper
changes enough to cause trouble for `Engine`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:StorageEngine/TSDB You know, for Metrics Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants