Skip to content

[ZDT migration] support root field addition#179595

Merged
pgayvallet merged 6 commits intoelastic:mainfrom
pgayvallet:kbn-179258-zdt-add-root-mappings
Apr 2, 2024
Merged

[ZDT migration] support root field addition#179595
pgayvallet merged 6 commits intoelastic:mainfrom
pgayvallet:kbn-179258-zdt-add-root-mappings

Conversation

@pgayvallet
Copy link
Copy Markdown
Contributor

@pgayvallet pgayvallet commented Mar 28, 2024

Summary

Fix #179258

Change the version compatibility and mapping generation (checkVersionCompatibility and generateAdditiveMappingDiff) of the ZDT migration algorithm to support the scenario were root fields are added (adding new fields to our base mappings)

@pgayvallet pgayvallet added Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// release_note:skip Skip the PR/issue when compiling release notes Feature:Migrations v8.14.0 labels Mar 28, 2024
@pgayvallet
Copy link
Copy Markdown
Contributor Author

/ci

@pgayvallet
Copy link
Copy Markdown
Contributor Author

/ci

@pgayvallet pgayvallet marked this pull request as ready for review March 28, 2024 13:25
@pgayvallet pgayvallet requested a review from a team as a code owner March 28, 2024 13:25
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-core (Team:Core)

} from '@kbn/core-saved-objects-server';
import {
IndexMappingMeta,
IndexMapping,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

NIT could be imported as a type

});
});

it('combines the changes from the types and from the root fields', () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we test behavior when adding properties to existing root fields, e.g. references?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not sure exactly how to do given helpers are stubbed for those unit tests, but I added one in 54d89d8, is that what you had in mind?

TBH this additive mapping diff generation is kinda unnecessary, we could just push everything as long as we know at least one change was done, but now that we have it...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, that's what I had in mind. The test covers it, thanks!

@gsoldevila
Copy link
Copy Markdown
Member

I don't know the zdt flow by heart, but what are the actions in case of 'greater'? is there a update + pickup of documents? How is the query built?

@pgayvallet
Copy link
Copy Markdown
Contributor Author

pgayvallet commented Mar 28, 2024

is there a update + pickup of documents? How is the query built?

Mapping update check and doc migration check are independent (init and documents_update_init stages respectively)

FWIW (given I know this is the underlying question 😄) I decided to not change anything in the document update phase given that adding new root mappings can't alter the document's content or trigger a data transformation in any way (I know we're doing it for the v2 impl but afaik this is still unnecessary). We will eventually change this if one day we want to add new root mappings that are backfilled.

EDIT: actually looking at the code, it does cause the migrator to enter the document update phase. But then no documents will be picked up given the query based on older model versions will not return any doc, so it's probably even better than not entering the phase.

const versionCheck = checkVersionCompatibility({
mappings: state.previousMappings,
types,
source: 'docVersions',
deletedTypes: context.deletedTypes,
});

@gsoldevila
Copy link
Copy Markdown
Member

gsoldevila commented Apr 2, 2024

EDIT: actually looking at the code, it does cause the migrator to enter the document update phase. But then no documents will be picked up given the query based on older model versions will not return any doc, so it's probably even better than not entering the phase.

Actually, what if we wanted to update all SO docs based on a new root type that was already in use but not yet mapped?
Also, the way the buildPickupMappingsQuery is build for ZDT is incorrect, as it assumes we build a query for certain types (not root fields). And in this case, the additiveMappingChanges contain root fields.

In v2 if the logic determines that root fields have been updated, it passes an undefined query, forcing the update of ALL documents.

@pgayvallet
Copy link
Copy Markdown
Contributor Author

Actually, what if we wanted to update all SO docs based on a new root type that was already in use but not yet mapped?

That's the thing: I don't think that's a valid use case.
We are in full control of root fields, given they can can only be defined by Core, and historically, we never used unindexed root fields, neither we're planning to (afaik).

Also, the way the buildPickupMappingsQuery is build for ZDT is incorrect, as it assumes we build a query for certain types (not root fields)

Because as previously said, we should never need to update / pickup types if only root fields changes, given they will never require a change in data. That's a mapping-exclusive operation.

@pgayvallet pgayvallet enabled auto-merge (squash) April 2, 2024 11:32
@pgayvallet pgayvallet merged commit f89c3e0 into elastic:main Apr 2, 2024
@kibana-ci
Copy link
Copy Markdown

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Apr 2, 2024
gsoldevila added a commit that referenced this pull request Apr 9, 2024
)

## Summary

Align V2 behavior with ZDT after
#179595

Under the assumption that whenever we want to add new root fields (e.g.
[created_by](#179344)), we will
systematically add mappings for them, we can skip the
`updateAndPickupMappings` operation iif ONLY root fields' mappings have
changed during an upgrade (aka if none of the SO types have updated
their mappings).

Up until now, the logic was updating ALL SO types whenever a `root`
field was added / updated.
This is expensive and unnecessary, and can cause a noticeable impact to
large customers during migrations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:Migrations release_note:skip Skip the PR/issue when compiling release notes Team:Core Platform Core services: plugins, logging, config, saved objects, http, ES client, i18n, etc t// v8.14.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ZDT Migration] support updating mapping for root fields

6 participants