Skip to content

[Transform] Auto-migrate max_page_search_size#119348

Merged
prwhelan merged 9 commits intoelastic:mainfrom
prwhelan:fix/1419
Jan 7, 2025
Merged

[Transform] Auto-migrate max_page_search_size#119348
prwhelan merged 9 commits intoelastic:mainfrom
prwhelan:fix/1419

Conversation

@prwhelan
Copy link
Member

@prwhelan prwhelan commented Dec 30, 2024

max_page_search_size had been deprecated in Pivot and migrated to Settings in ES 7.x. We are removing the key in Pivot in 9.x. It is still possible to create Pivots with max_page_search_size, though a deprecation is logged. To prep for its removal, this change automatically migrates max_page_search_size from Pivot to Settings as if the user called _update.

  • PUT _transform with max_page_search_size in Pivot will migrate the value to Settings as part of the Transform's creation. The user will receive an additional deprecation warning informing of this migration.
  • Existing transforms with max_page_search_size in Pivot will migrate the value to Settings as part of node assignment during start. Transform Messages and Elasticsearch logs will record this event so users will know what happened.
@prwhelan
[Transform] Auto-migrate max_page_search_size
365f0a2 
`max_page_search_size` had been deprecated in Pivot and migrated to
Settings in ES 7.x.  We are removing the key in Pivot in 9.x.  It is
still possible to create Pivots with `max_page_search_size`, though a
deprecation is logged.  To prep for its removal, this change
automatically migrates `max_page_search_size` from Pivot to Settings as
if the user called `_update`.

- `PUT _transform` with `max_page_search_size` in Pivot will migrate the
  value to Settings as part of the Transform's creation.  The user will
  receive an additional deprecation warning informing of this migration.
- Existing transforms with `max_page_search_size` in Pivot will migrate
  the value to Settings as part of node assignment during start.
  Transform Messages and Elasticsearch logs will record this event so
  users will know what happened.
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Dec 30, 2024

Hi @prwhelan, I've created a changelog YAML for you.
@prwhelan prwhelan marked this pull request as ready for review December 30, 2024 17:23
@elasticsearchmachine elasticsearchmachine added the Team:ML Meta label for the ML team label Dec 30, 2024
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Dec 30, 2024

Pinging @elastic/ml-core (Team:ML)
dan-rubinstein
dan-rubinstein reviewed Jan 3, 2025
View reviewed changes
...rc/javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformPivotRestIT.java Outdated
assertThat(createTransformResponse.get("acknowledged"), equalTo(Boolean.TRUE));

Map<String, Object> transform = getTransformConfig(transformId, BASIC_AUTH_VALUE_TRANSFORM_ADMIN_WITH_SOME_DATA_ACCESS);
assertThat(XContentMapValues.extractValue("pivot.max_page_search_size", transform), equalTo(null));
Copy link
Member

@dan-rubinstein dan-rubinstein Jan 3, 2025

Choose a reason for hiding this comment

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

Minor: Can just use assertNull here.
.../transform/src/main/java/org/elasticsearch/xpack/transform/TransformConfigAutoMigration.java Outdated
.withThrowable(validationException)
.log(
"Failed to validate auto-migrated Config. Please use the _update API to correct and update "
+ "the Transform configuration. Continuing with old config."
Copy link
Member

@dan-rubinstein dan-rubinstein Jan 3, 2025

Choose a reason for hiding this comment

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

Minor: Extra whitespace in between sentences.
...form/src/main/java/org/elasticsearch/xpack/transform/action/TransportPutTransformAction.java Outdated
private TransformConfig autoMigrateConfig(Request request) {
var config = transformConfigAutoMigration.migrate(request.getConfig());
if (config != request.getConfig()) {
deprecationLogger.warn(
Copy link
Member

@dan-rubinstein dan-rubinstein Jan 3, 2025

Choose a reason for hiding this comment

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

Could this logging logic be contained within the transformConfigAutoMigration.migrate(...) function logic? Seems like something we'd want to happen whenever we call migrate and adding the logic there instead would also add a deprecation log when we call migrateAndSave.
...sform/src/test/java/org/elasticsearch/xpack/transform/TransformConfigAutoMigrationTests.java
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;

public class TransformConfigAutoMigrationTests extends ESTestCase {
Copy link
Member

@dan-rubinstein dan-rubinstein Jan 3, 2025

Choose a reason for hiding this comment

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

Can we add a test for when the transform has a max_page_search_size in both pivot config and settings and we just set the value to null? I know it's not a case we expect to actually hit but the code does specifically has this as an implementation decision we made so it's nice to have a test for it so we don't accidentally change this behavior.
@prwhelan
Copy link
Member Author

prwhelan commented Jan 3, 2025

@elasticmachine update branch
dan-rubinstein
dan-rubinstein approved these changes Jan 3, 2025
View reviewed changes
Copy link
Member

@dan-rubinstein dan-rubinstein left a comment

Choose a reason for hiding this comment

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

LGTM!
@prwhelan
Copy link
Member Author

prwhelan commented Jan 7, 2025

@elasticmachine update branch
@prwhelan
Copy link
Member Author

prwhelan commented Jan 7, 2025

@elasticmachine update branch
@prwhelan
Copy link
Member Author

prwhelan commented Jan 7, 2025

Verified with latest changes, settings max_page_search_size in PUT:

ElasticsearchWarning: Automatically migrating [max_page_search_size] from the pivot to the transform setting.

{
  "_index": ".ds-metrics-apm.app.elasticsearch-default-2025.01.07-000001",
  "_version": 1,
  "_source": {
    "@timestamp": "2025-01-07T17:09:04.516Z",
    "agent": {
      "activation_method": "javaagent-flag",
      "ephemeral_id": "<>",
      "name": "java",
      "version": "1.52.0"
    },
    "data_stream": {
      "dataset": "apm.app.elasticsearch",
      "namespace": "default",
      "type": "metrics"
    },
    "es": {
      "transform": {
        "automigration": {
          "count": {
            "total": 1
          }
        }
      },
  }
}
@prwhelan prwhelan enabled auto-merge (squash) January 7, 2025 17:15
@prwhelan prwhelan merged commit 7262fb3 into elastic:main Jan 7, 2025
prwhelan added a commit to prwhelan/elasticsearch that referenced this pull request Jan 7, 2025
@prwhelan
[Transform] Auto-migrate max_page_search_size (elastic#119348)
8881018 
`max_page_search_size` had been deprecated in Pivot and migrated to
Settings in ES 7.x.  We are removing the key in Pivot in 9.x.  It is
still possible to create Pivots with `max_page_search_size`, though a
deprecation is logged.  To prep for its removal, this change
automatically migrates `max_page_search_size` from Pivot to Settings as
if the user called `_update`.

- `PUT _transform` with `max_page_search_size` in Pivot will migrate the
  value to Settings as part of the Transform's creation.  The user will
  receive an additional deprecation warning informing of this migration.
- Existing transforms with `max_page_search_size` in Pivot will migrate
  the value to Settings as part of node assignment during start.
  Transform Messages and Elasticsearch logs will record this event so
  users will know what happened.
prwhelan added a commit that referenced this pull request Jan 7, 2025
@prwhelan
[Transform] Auto-migrate max_page_search_size (#119348) (#119697)
40b1e3e 
`max_page_search_size` had been deprecated in Pivot and migrated to
Settings in ES 7.x.  We are removing the key in Pivot in 9.x.  It is
still possible to create Pivots with `max_page_search_size`, though a
deprecation is logged.  To prep for its removal, this change
automatically migrates `max_page_search_size` from Pivot to Settings as
if the user called `_update`.

- `PUT _transform` with `max_page_search_size` in Pivot will migrate the
  value to Settings as part of the Transform's creation.  The user will
  receive an additional deprecation warning informing of this migration.
- Existing transforms with `max_page_search_size` in Pivot will migrate
  the value to Settings as part of node assignment during start.
  Transform Messages and Elasticsearch logs will record this event so
  users will know what happened.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement :ml/Transform Transform Team:ML Meta label for the ML team v8.18.0 v9.0.0

4 participants

@prwhelan @elasticsearchmachine @dan-rubinstein @elasticmachine