-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Script: Reindex & UpdateByQuery Metadata #88665
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
Script: Reindex & UpdateByQuery Metadata #88665
Conversation
Adds metadata classes for Reindex and UpdateByQuery contexts. For Reindex metadata: * _index can't be null * _id, _routing and _version are writable and nullable * _now is read-only * op is read-write must be 'noop', 'index' or 'delete' Reindex metadata keeps the originx value for _index, _id, _routing and _version so that `Reindexer` can see if they've changed. If _version is null in the ctx map, or, equivalently, the augmentation `setVersionToInternal()` was called by the script, `Reindexer` sets document versioning to internal. If `_version` is `null` in the ctx map, `getVersion` returns `Long.MIN_VALUE`. For UpdateByQuery metadata: * _index, _id, _version, _routing are all read-only * _routing is also nullable * _now is read-only * op is read-write and one of 'index', 'noop', 'delete' Closes: elastic#86472
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Hi @stu-elastic, I've created a changelog YAML for you. |
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.
@stu-elastic Did a first pass. Looks pretty good. Given that I'm still getting up to speed a bit here, I would've had an easier time if FieldProperty refactorings was a separate PR.
Also please take a look to see if additional validation of source wrapper is indeed a bug fix because if so that will need to be a separate PR. I made a comment in AbstractAsyncBulkByScollAction about this.
server/src/main/java/org/elasticsearch/ingest/IngestDocMetadata.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/script/ReindexMetadata.java
Outdated
Show resolved
Hide resolved
modules/reindex/src/main/java/org/elasticsearch/reindex/AbstractAsyncBulkByScrollAction.java
Show resolved
Hide resolved
modules/reindex/src/main/java/org/elasticsearch/reindex/AbstractAsyncBulkByScrollAction.java
Outdated
Show resolved
Hide resolved
modules/reindex/src/main/java/org/elasticsearch/reindex/AbstractAsyncBulkByScrollAction.java
Outdated
Show resolved
Hide resolved
modules/reindex/src/main/java/org/elasticsearch/reindex/AbstractAsyncBulkByScrollAction.java
Show resolved
Hide resolved
modules/reindex/src/main/java/org/elasticsearch/reindex/Reindexer.java
Outdated
Show resolved
Hide resolved
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.
LGTM. A couple more minor comments.
public abstract static class ScriptApplier<T extends Metadata> | ||
implements | ||
BiFunction<RequestWrapper<?>, ScrollableHitSource.Hit, RequestWrapper<?>> { | ||
protected static final String INDEX = "index"; |
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.
Would you please add a comment here on what this particular INDEX represents. I believe it's the op, right?
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.
Done.
public String toString() { | ||
return id.toLowerCase(Locale.ROOT); | ||
} | ||
protected abstract void updateRequest(RequestWrapper<?> request, T metadata); |
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.
Please move this above requestFromOp just to follow the same pattern as execute.
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.
Done.
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.
Looked again, and still LGTM! Thanks.
* upstream/main: Add 8.5 migration docs (elastic#88923) Script: Reindex & UpdateByQuery Metadata (elastic#88665) Remove unused plugins dir var from server CLI (elastic#88917) Use tracing API in TaskManager (elastic#88885) Add source fallback for keyword fields using operation (elastic#88735) Prune changelogs after 8.3.3 release Bump versions after 8.3.3 release Add a test for checking for misspelled "dry_run" parameters for Desired Nodes API (elastic#88898) Speedup BalanceUnbalancedClusterTests (elastic#88794) Preventing exceptions on node shutdown in integration tests (elastic#88827) Do not trigger check part3 for test mute and docs PRs (elastic#88895) Add troubleshooting docs about data corruption (elastic#88760) Mute RollupActionSingleNodeTests#testRollupDatastream (elastic#88891) [DOCS] Domain splitting impacts API keys (elastic#88677) Fix SqlSearchIT testAllTypesWithRequestToOldNodes (elastic#88866) (elastic#88883) Update synthetic-source.asciidoc (elastic#88880) Log more details in TaskAssertions (elastic#88864) Make Tuple a record (elastic#88280)
Adds metadata classes for Reindex and UpdateByQuery contexts.
For Reindex metadata:
Reindex metadata keeps the originx value for _index, _id, _routing and _version
so that
Reindexer
can see if they've changed.If _version is null in the ctx map, or, equivalently, the augmentation
setVersionToInternal()
was called by the script,Reindexer
sets documentversioning to internal. If
_version
isnull
in the ctx map,getVersion
returns
Long.MIN_VALUE
.For UpdateByQuery metadata:
Closes: #86472