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] Refine Transform Destination Index message #122192

Merged
merged 5 commits into from
Feb 13, 2025

Conversation

prwhelan
Copy link
Member

When we detect that a Transform writes to the index and the index is incompatible with the next version, change the message, detail, and URL to help the user take the necessary steps to migrate the destination index.

When we detect that a Transform writes to the index and the index is
incompatible with the next version, change the message, detail, and
URL to help the user take the necessary steps to migrate the destination
index.
@prwhelan prwhelan added >non-issue :ml/Transform Transform Team:ML Meta label for the ML team auto-backport Automatically create backport pull requests when merged v9.0.0 v8.18.0 v8.18.1 v9.1.0 labels Feb 10, 2025
@prwhelan prwhelan marked this pull request as ready for review February 10, 2025 18:07
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

var transforms = transformIdsForIndex(indexMetadata, indexToTransformIds);
if (transforms.isEmpty() == false) {
return new DeprecationIssue(
DeprecationIssue.Level.WARNING,
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 is probably fine as a warning - this message is emitted when there is a write block on the index, which will pause the transform indefinitely. The Transform will be effectively disabled anyway, and the user should probably delete or reset the Transform (which we can put in the link)

Copy link
Contributor

@przemekwitek przemekwitek left a comment

Choose a reason for hiding this comment

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

LGTM

Please add some verb to the PR title e.g.: "Refine Transform destination index message"?
Also, I cannot see the file with PR title generated. Do you know why it could be missing?

} else {
return new DeprecationIssue(
DeprecationIssue.Level.WARNING,
"Old index with a compatibility version < 9.0 Has Been Ignored",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why capital letters in "Has Been Ignored"?

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 do not know, that's what previously existed. We can edit it probably?

"https://www.elastic.co/guide/en/elasticsearch/reference/master/migrating-9.0.html"
+ "#breaking_90_transform_destination_index",
Strings.format(
"Transforms [%s] write to this index with version [%s] and cannot be supported as read-only in 9.0",
Copy link
Contributor

@rudolf rudolf Feb 11, 2025

Choose a reason for hiding this comment

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

I'm not sure a user would know what to do after reading this. It sounds a bit like if they upgrade to 9.0 their cluster might break because this incompatible index isn't ignored and "cannot be supported as read-only in 9.0" Whereas I think we want the takeaway from this message to be "it's fine, you can upgrade, but just beaware that your transforms aren't working"

Maybe something like:

This read-only index was created in version 7.17 and will be supported as a read-only index in 9.0. The following transforms are no longer able to write to this index [%s].

Copy link
Contributor

@szabosteve szabosteve Feb 11, 2025

Choose a reason for hiding this comment

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

I agree to change the messaging based on @rudolf's input above. I'm not familiar with the subject, but would it be possible to add something actionable to the end of the message, something that the user can do to mitigate/solve the issue? For example:

This index was created in version 7.17 and will be supported as a read-only index in 9.0. The following transforms are no longer able to write to this index [%s]. Refer to the migration guide to learn more about how to handle your transforms destination indices."

@prwhelan prwhelan changed the title [Deprecation] Transform Destination Index message [Deprecation] Refine Transform Destination Index message Feb 11, 2025
Copy link
Contributor

@szabosteve szabosteve left a comment

Choose a reason for hiding this comment

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

Copy LGTM!

"https://www.elastic.co/guide/en/elasticsearch/reference/master/migrating-9.0.html"
+ "#breaking_90_transform_destination_index",
Strings.format(
"This index was created in version [%s] and will be supported as a read-only index in 9.0. The following "
Copy link
Contributor

Choose a reason for hiding this comment

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

will be supported as a read-only index in 9.0

if (transforms.isEmpty() == false) { doesn't mean this index is read-only. As a CRITICAL deprecation the user still needs to do something to make this index safe for upgrade.

Copy link
Member Author

Choose a reason for hiding this comment

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

We spoke over slack, I'll summarize the discussion here:

I agree that the messaging doesn't quite make sense. We reworded it to highlight:

  1. the index was created in version %s
  2. this index is not compatible in the next version of elasticsearch, and the user needs to take some action before upgrading or else the cluster will break
  3. there are transforms writing to this index, so the user will need to take different or additional actions to resolve this for transforms

Much of what we can say will go in the supplemental document supplied by the URL.

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

🥳

@prwhelan prwhelan merged commit fda7fc7 into elastic:main Feb 13, 2025
17 checks passed
prwhelan added a commit to prwhelan/elasticsearch that referenced this pull request Feb 13, 2025
)

When we detect that a Transform writes to the index and the index is
incompatible with the next version, change the message, detail, and
URL to help the user take the necessary steps to migrate the destination
index.
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
9.0
8.18 Commit could not be cherrypicked due to conflicts

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

elasticsearchmachine pushed a commit that referenced this pull request Feb 13, 2025
…122509)

When we detect that a Transform writes to the index and the index is
incompatible with the next version, change the message, detail, and
URL to help the user take the necessary steps to migrate the destination
index.
prwhelan added a commit to prwhelan/elasticsearch that referenced this pull request Feb 13, 2025
)

When we detect that a Transform writes to the index and the index is
incompatible with the next version, change the message, detail, and
URL to help the user take the necessary steps to migrate the destination
index.
prwhelan added a commit that referenced this pull request Feb 14, 2025
…) (#122524)

When we detect that a Transform writes to the index and the index is
incompatible with the next version, change the message, detail, and
URL to help the user take the necessary steps to migrate the destination
index.
prwhelan added a commit to prwhelan/elasticsearch that referenced this pull request Feb 14, 2025
…ic#122192) (elastic#122524)

When we detect that a Transform writes to the index and the index is
incompatible with the next version, change the message, detail, and
URL to help the user take the necessary steps to migrate the destination
index.
elasticsearchmachine pushed a commit that referenced this pull request Feb 14, 2025
…) (#122524) (#122648)

When we detect that a Transform writes to the index and the index is
incompatible with the next version, change the message, detail, and
URL to help the user take the necessary steps to migrate the destination
index.
jloleysens added a commit to elastic/kibana that referenced this pull request Feb 17, 2025
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 18, 2025
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 :ml/Transform Transform >non-issue Team:ML Meta label for the ML team v8.18.0 v8.18.1 v8.19.0 v9.0.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants