-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Ingest Manager] rollover data stream when index template mappings are not compatible #69180
[Ingest Manager] rollover data stream when index template mappings are not compatible #69180
Conversation
Pinging @elastic/ingest-management (Team:Ingest Management) |
If the code is decoupled from specific packages (which I think it mostly is) you could also test without a package at the moment. What I had in mind: Flow with compatible mapping
Flow with incompatible mapping
The above does not mean we should not test with packages too in the future, but I think we can decouple this at the moment to have more local tests. |
What I was thinking is this is only called within the installPackage handler from the install endpoint eg: |
@neptunian Not sure I follow what this means for the proposed tests. Does it mean we can't do it at the moment or that we should add it directly to the integrations tests? |
Looking closer after our conversation, I don't think I can test these functions outside of the API endpoint. They all require an argument for the ES callCluster for the current user which is passed to the request handlers when making the request to the endpoint through the API. I think this is why you had mentioned needing to create an endpoint for integration tests with ES. |
@elasticmachine merge upstream |
@neptunian I suggest we get this PR merged rather soonish and can still follow up with tests. This will also allow us to test it manually. |
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
path, | ||
}); | ||
} catch (error) { | ||
throw new Error(`cannot rollover data stream ${dataStreamName}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding the error message that comes from elasticsearch
to the new error message? This can be helpful for troubleshooting.
At some point, when we touch code, I think it would be a good idea to implement error handling as outlined in #66688 (as in: use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested whether the actual rollover happens, as you already agreed to add tests in a follow-up PR. Code looks good, and nothing breaks in local testing, so 👍!
…e not compatible (elastic#69180) * rollover data stream when index template mappings are not compatible * update error messages Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
…e not compatible (elastic#69180) * rollover data stream when index template mappings are not compatible * update error messages Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
#68381
During an update, updating the current write index's mappings could be incompatible if a mapping type has changed, at which point a rollover of the index should take place.
It's not possible to easily test without having packages to update to in the registry that have incompatible mappings. Otherwise code changes would be necessary, so this should perhaps wait to be merged until those test packages are ready and I can include an integration test (#67372)