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

Deprecation info for joda-java migration on 7.x #42659

Merged
merged 13 commits into from
Jun 5, 2019

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented May 29, 2019

Some clusters might have been already migrated to version 7 without being warned about the joda-java migration changes.
Deprecation api on that version will give them guidance on what patterns need to be changed.
relates. This change is using the same logic like in 6.x that is: verifying the pattern is from the incompatible set ('y'-Y', 'C', 'Z' etc), not from predifined set, not prefixed with 8. AND was also created in 6.x. Mappings created in 7.x are considered migrated and should not generate warnings

#42010

@pgomulka pgomulka added :Core/Infra/Core Core issues without another label 7x labels May 29, 2019
@pgomulka pgomulka self-assigned this May 29, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@pgomulka pgomulka marked this pull request as ready for review May 30, 2019 19:36
@pgomulka pgomulka requested a review from gwbrown May 30, 2019 19:39
}

@SuppressWarnings("unchecked")
static DeprecationIssue checkFormatOnPipeline(ClusterState state) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

any ideas how to check that pipeline was created in 6.x?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's possible to check when the pipeline was created, unfortunately. However, it's much easier to update pipelines to use the 8 prefix (compared to mappings) so I don't think it matters as much here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gwbrown agree, it is easier to fix them (just a PUT with a fixed pipeline). Should we then alert for all pipelines without the 8prefix, suspecting that it was not updated?
Or should we not alert at all and let user find this on their own?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users in 7 are not required to prefix their format with 8.
If we just alert without checking the version, and someone changes their pattern from Y to y it will still alert from him. Both are deprecated in 6.x. (Y->y and y->u)
I wonder if the best way here is not alert about pipelines at all in 7x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we discussed this offline and decided to remove pipelines' checks in 7.x. There would be no easy way to fix these deprecation warnings and this is likely to generate a lot of false positives.

@pgomulka
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/bwc

@pgomulka
Copy link
Contributor Author

pgomulka commented Jun 3, 2019

@elasticmachine update branch

@pgomulka
Copy link
Contributor Author

pgomulka commented Jun 3, 2019

@elasticmachine ok to test

Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

LGTM once CI is happy.

Left one minor comment on the tests, but no need for another round of review after addressing unless you make other substantial changes.

PutPipelineRequest pipelineRequest = new PutPipelineRequest("testingestpipeline",
new BytesArray("{\n" +
" \"description\": \"Pipeline with date and date_index_name processor\",\n" +
" \"processors\": [\n" +
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a date processor to this that doesn't use deprecated formats to help make this more resistant to regressions.

@pgomulka pgomulka merged commit ab5bc83 into elastic:7.x Jun 5, 2019
pgomulka added a commit to pgomulka/elasticsearch that referenced this pull request Jun 10, 2019
 Some clusters might have been already migrated to version 7 without being warned about the joda-java migration changes.
Deprecation api on that version will give them guidance on what patterns need to be changed.
relates. This change is using the same logic like in 6.8 that is: verifying the pattern is from the incompatible set ('y'-Y', 'C', 'Z' etc), not from predifined set, not prefixed with 8. AND was also created in 6.x. Mappings created in 7.x are considered migrated and should not generate warnings

There is no pipeline check (present on 6.8) as it is impossible to verify when the pipeline was created, and therefore to make sure the format is depracated or not
@jpountz jpountz removed 7x :Core/Infra/Core Core issues without another label labels Jul 5, 2019
@jpountz jpountz added :Core/Infra/Core Core issues without another label >enhancement labels Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Core Core issues without another label >enhancement v7.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants