Slice: composite IDs to ensure uniqueness within a slice#151686
Slice: composite IDs to ensure uniqueness within a slice#151686benwtrent wants to merge 68 commits into
Conversation
…lice_id # Conflicts: # server/src/main/java/org/elasticsearch/action/get/MultiGetRequest.java # server/src/main/java/org/elasticsearch/action/get/TransportMultiGetAction.java # server/src/main/java/org/elasticsearch/index/engine/LuceneChangesSnapshot.java # server/src/main/java/org/elasticsearch/index/engine/LuceneSyntheticSourceChangesSnapshot.java # server/src/main/java/org/elasticsearch/index/mapper/IdFieldMapper.java # server/src/main/java/org/elasticsearch/index/mapper/IdLoader.java # server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java
There was a problem hiding this comment.
Took a pass through the composite-_id change. Overall looks good — the term-space disjointness holds for all id types, delete/CCR/translog carry the compound correctly, and the TermsSliceQuery slicing only hashing the 0x00-terminated search term is a clean fix. The UidTests round-trip/disjointness coverage is great too.
A few things inline — mostly the "identity lookup with no routing" cases you already called out in the description.
One small test gap: a per-slice if_seq_no/if_primary_term conflict test would be nice to lock the core contract.
| public static BytesRef encodeCompoundId(String id, String slice) { | ||
| BytesRef encodedId = encodeId(id); | ||
| byte[] sliceBytes = slice.getBytes(StandardCharsets.UTF_8); | ||
| assert sliceBytes.length <= 128 : "slice length [" + sliceBytes.length + "] exceeds 128"; |
There was a problem hiding this comment.
The <= 128 here is just an assert, so with assertions off a slice longer than 128 bytes would silently wrap the trailing length byte and corrupt the term. It's enforced in SliceIndexing.validateUserSliceValue today, but since this is the on-disk encoding it might be worth a hard IllegalArgumentException here too, plus a test with a max-length slice.
|
Not sure if this is relevant but I want to call it out. I know we have now the concept of synthetic id, is this change conflicting in that case? Do we need to forbid (for now) using slices in that case? |
|
@iverase we will need to add special handling for synthetic IDs and slicing and the combo shouldn't be allowed right now. I will confirm if it is disallowed. if not, let's block it. |
Needed before we encode ID information for slices. Effectively requires _slice to be provided on slice enabled indices. related: #151686
Its only caller is DocumentParserContext, in the same package.
TSDB derives _id from the _tsid and @timestamp in postParse and copies it to nested documents there, asserting they start without an _id. The generic propagation now runs first and, since context.id() is already set, added an _id to those documents and tripped the assertion. Opt the TSDB _id mapper out of the generic path.
The columnar-mode lookup was duplicated in MappingLookup and IdLoader.
Covers index, get, search and delete scoped by _slice: the same _id in two slices stays distinct, responses return the plain _id, and overwrite and delete only affect their own slice. Also declares the _slice param on the delete API spec so the test can pass it.
index.slice_indexing is behind a feature flag, so it is absent from the build-time feature metadata on release builds. There the yaml runner errors on the requires cluster_features check instead of skipping, which red release-tests. A rest-api-spec test cannot gate on a flagged feature without wiring the flag into the metadata extractor and the test cluster. End-to-end coverage stays in SliceCompositeIdIT and SliceFollowingIT.
Covers index, get, search and delete scoped by _slice: the same _id in two slices stays distinct, responses return the plain _id, and overwrite and delete only affect their own slice. The test gates on a runtime slice_indexing capability rather than a cluster feature, so it skips on builds where the flag is off instead of erroring on the build-time feature metadata. Advertise that capability from the create-index action when the flag is on, and declare the _slice param on the delete API spec.
henningandersen
left a comment
There was a problem hiding this comment.
Spent some time on this. It is a big PR with a few different concerns in it, I wonder if it could be split?
There are a few testing gaps (noted the ones I found).
I also wonder about the use of routing throughout slicing. It is similar but not the same. This PR takes that to a new level with the id encoding. So now routing is sometimes appended to the _id, which is sort of confusing. I think we should at least plan to clean this up to make the two concepts mutually exclusive.
IIUC, it does shine through on search-hits, where slices are exposed as routing. I think we need to fix that before making slices available. Also, I think we need to have slice available on search hit. It is not clear to me that it is (since if it is, I'd expect some of the new tests to have validated this).
I am probably not done with my review, but wanted to push these comments now and then wait for feedback on them.
| final long autoGeneratedIdTimestamp = -1; | ||
| op = new Translog.Index(id, seqNo, primaryTerm, version, source, routing, autoGeneratedIdTimestamp); | ||
| op = new Translog.Index( | ||
| BytesRef.deepCopyOf(idBytes), |
There was a problem hiding this comment.
Why deepCopy here?
There was a problem hiding this comment.
Dropped it — the bytes are already stable here, so the copy was redundant.
| public void testSliceDeleteRecoversCompoundUid() throws Exception { | ||
| final String id = "doc-1"; | ||
| final String slice = "slice-7"; | ||
| // A delete is always recorded as a tombstone (even of an absent doc) so history/recovery can replay it. The engine |
There was a problem hiding this comment.
I think this test should ideally have two existing docs with different slices, ensuring that the delete does not lookup the data by id and pick the wrong slice from that data.
There was a problem hiding this comment.
Added testSliceDeleteAmongSameIdDifferentSlicesRecoversCorrectSlice for exactly this: two live docs share the id in different slices, we delete one, and assert the recovered tombstone carries that slice (not the other). Mutation-checked — flipping the expected slice fails.
| * holding only a {@link MappedFieldType} still decode with the right encoding. | ||
| */ | ||
| public static String decodeStoredId(MappedFieldType fieldType, byte[] value) { | ||
| return fieldType instanceof AbstractIdFieldType idFieldType ? idFieldType.decodeStoredId(value) : Uid.decodeId(value); |
There was a problem hiding this comment.
How can this not be an abstract id field type?
There was a problem hiding this comment.
Yeah, it always is. Switched to an assert + direct cast to AbstractIdFieldType instead of the instanceof fallback.
| createSliceIndex("upd", 1); | ||
| indexDoc("upd", "sa", "1", "va"); | ||
| indexDoc("upd", "sb", "1", "vb"); | ||
| refresh("upd"); |
There was a problem hiding this comment.
I think we need this both with and without the refresh? To check the live version map lookup works. Possibly also to have both data in lucene and live version map.
There was a problem hiding this comment.
Done — added a random refresh so it covers both the live version map and Lucene.
| assertResponse(searchSlice("idx", "sa", QueryBuilders.matchAllQuery()), r -> { | ||
| assertThat(r.getHits().getTotalHits().value(), equalTo(1L)); | ||
| assertThat(r.getHits().getAt(0).getId(), equalTo("1")); | ||
| assertThat(r.getHits().getAt(0).getSourceAsMap().get("field"), equalTo("va")); |
There was a problem hiding this comment.
Can we assert on the slice too?
It is not clear to me if slice is available in the search response. The classic search, then GET needs that I think.
Also, I think routing is returned from searc, which it should ideally not be. Sort of separate concern, though getting slice into the search result relates to the work in this PR.
There was a problem hiding this comment.
Good catch on both. Hits now return _slice and no longer _routing — re-keyed in FetchFieldsPhase for slice indices. The test asserts _slice equals the slice and _routing is null, so search-then-GET has what it needs.
| indexDoc("rec", "sb", "1", "vb"); | ||
| // Delete only slice sa's document. Intentionally do not flush, so the index+delete ops are replayed from the translog | ||
| // on recovery — exercising the delete-translog-replay path against the composite term. | ||
| deleteDoc("rec", "sa", "1"); |
There was a problem hiding this comment.
Can we add a 3rd doc that is updated/replaced too?
There was a problem hiding this comment.
Added — sa/2 is replaced after the commit and asserted.
| GetResponse gb = getDoc("rt", "sb", "1"); | ||
| assertThat(gb.isExists(), equalTo(true)); | ||
| assertThat(gb.getSource().get("field"), equalTo("vb")); | ||
| } |
There was a problem hiding this comment.
Can we verify that it indeed did read from translog. I worry it might not since we do not track translog locations initially.
There was a problem hiding this comment.
GET runs with no refresh so it's realtime and resolves the right slice + plain id, which we assert. We don't pin the translog location specifically — shout if you'd like that made explicit.
There was a problem hiding this comment.
I think we could be hitting the fallback path here so I'd like to add verification that we do not, for instance checking that no refresh happened or some other stat.
There was a problem hiding this comment.
Added verification. The test now does a warm-up GET first, which flips the live version map to safe access and starts tracking translog locations, then asserts refreshCount is unchanged across the slice GETs — so they are served from the translog rather than the getFromSearcher refresh fallback you linked.
| // The same user id in two slices yields two independent documents, each created (not an update of the other). | ||
| assertThat(a.getResult(), equalTo(DocWriteResponse.Result.CREATED)); | ||
| assertThat(b.getResult(), equalTo(DocWriteResponse.Result.CREATED)); | ||
|
|
There was a problem hiding this comment.
Should we randomly refresh here to hit both the version map and the lucene index cases?
| ).actionGet(); | ||
| assertThat(updatedB.getResult(), equalTo(DocWriteResponse.Result.UPDATED)); | ||
|
|
||
| refresh("occ"); |
There was a problem hiding this comment.
Should we randomize this refresh too?
| * Slice-scoped index and delete ops must replicate correctly: after the primary is stopped and the replica (which | ||
| * received the ops via replication) is promoted, the promoted copy reflects that the delete hit only its slice. | ||
| */ | ||
| public void testSliceDocsSurviveReplicaPromotion() throws Exception { |
There was a problem hiding this comment.
I think we are missing an ops based peer recovery with slices. Also, a relocation with slices seems to be missing (though it is sort of similar to regular replication, it does do some catch up too). And establishing a new replica with indexing outstanding too. And finally, a full copy peer recovery.
There was a problem hiding this comment.
Added replica promotion here. The fuller matrix — ops-based peer recovery, relocation, a new replica with outstanding indexing, and full-copy peer recovery — is still open; planning it as a follow-up unless you'd rather it block this.
There was a problem hiding this comment.
As long as we do not enable slice without it, I am ok to split it up.
There was a problem hiding this comment.
Update: added these directly in this PR rather than deferring. SliceCompositeIdIT now covers ops-based and file-based peer recovery, shard relocation, and establishing a new replica with outstanding indexing — each asserting that a delete hit only its slice's compound term (leaving the same id in another slice intact) on the recovered copy. Slice still stays behind the off-by-default slice_indexing flag.
Re-key the internal routing value as _slice on search hits of a slice-enabled index so _routing is never exposed and the slice is available for search-then-get. Also decode the stored _id through the _id field type and drop a redundant _id copy in the changes snapshot.
Assert hits expose _slice and never _routing, recover a delete among same-id docs in different slices, randomly refresh to cover the version map and the index, and replace/delete after a commit on full restart. Add a TermsSliceQuery test that a slice-enabled _id partitions every document into exactly one scroll slice.
The _slice parameter was documented under the always-on index_mode feature flag, exposing it even where slice indexing is disabled. Point it at es.slice_indexing_feature_flag_enabled so it tracks the feature.
Route both decodeCompoundId and the document-mode id loader through a single stripSlice helper so the split logic lives in one place.
|
Addressed the review in 4 commits on top of the merge (keeping it as one PR for now rather than splitting):
Inline replies below. |
A slice-enabled index surfaces its routing value as _slice, so reindex, update-by-query and delete-by-query must read _slice when _routing is absent to scope their per-hit sub-requests to the right slice.
Drop the FetchFieldsPhase re-key that moved _routing into a _slice metadata field: main already exposes _slice as a fetchable alias of _routing, so the re-key double-surfaced it (in the wrong map) and hid _routing from reindex, delete-by-query and update-by-query. Reverting restores _routing for those paths; the IT now fetches _slice explicitly.
|
@henningandersen — following your point that routing and slicing were getting mixed, and that slice is no longer "just routing + index sorting" now that it participates in uniqueness, pushed two commits to clean that up:
Green across the slice suites: unit, engine, |
Model a document identity as a Uid value carrying its id, optional slice and the term indexed into _id. Without slicing the term is encodeId(id) and the slice is null, so id and uid coincide; with slicing it is the compound id#slice. This replaces the scattered id<->term conversions and the sliceEnabled ? compound : plain branches with Uid.create/fromTerm, and lets Engine.Get carry a Uid instead of a raw id plus term.
e9f028f to
5bfd6cb
Compare
5bfd6cb to
561bc4f
Compare
A slice-enabled index no longer exposes _routing anywhere: it is hidden from field retrieval (SearchExecutionContext#getMatchingFieldNames) and GET responses, and the slice is surfaced as the _slice field instead - by default in search hits and GET, routing stays purely internal. Its field type stays resolvable so slice routing filters still work. Update and the reindex/by-query hit source read _slice as the routing value, and the _slice parameter is added to the create and update REST specs.
561bc4f to
bf32f30
Compare
henningandersen
left a comment
There was a problem hiding this comment.
This looks mostly good to me, left a few comments. This should not block on my approval, but I think we should have someone from storage engine team review the mapping and search changes as well as the PR in entirety. I did not have time to be very thorough, but I like the simplifications done in this new version.
| id: "1" | ||
| _slice: r1 | ||
| - match: { _routing: r1 } | ||
| - match: { fields._slice.0: r1 } |
There was a problem hiding this comment.
I am not sure why this is in fields here? I think fields normally need explicit enablement?
There was a problem hiding this comment.
Good catch — _slice shouldn't have been under fields. It's now surfaced as a top-level metadata field (like _routing), so it renders at the top level without explicit enablement; an explicit fields: ["_slice"] still works and lands under fields like other metadata.
| }, | ||
| "_slice": { | ||
| "type": "string", | ||
| "description": "Slice routing value, used instead of routing when the index has index.slice.enabled set to true", |
There was a problem hiding this comment.
nit: I am not sure I like the description here, since slice is more than routing? But it is a nit for sure.
There was a problem hiding this comment.
Reworded — the description now presents slice as the slice identifier for a slice-enabled index rather than framing it as a replacement for routing.
| "type": "list", | ||
| "description": "Specific routing value" | ||
| }, | ||
| "_slice": { |
There was a problem hiding this comment.
I do notice we use _slice for the parameter value everywhere, but the _ seems slightly strange? Maybe OK because users can filter on _slice and it is a metadata field. Not requesting any change here, more a question about to clarify the reasoning for it. For instance, routing does not have this.
There was a problem hiding this comment.
Agreed, and thanks for flagging it. I've renamed the request parameter to slice (no underscore, matching routing) and kept _slice only for the metadata field and query/output (matching _routing). So request inputs mirror routing and the field mirrors _routing.
| index: slice_index | ||
| _slice: b | ||
| body: { query: { ids: { values: ["1"] } } } | ||
| - match: { hits.total.value: 1 } |
There was a problem hiding this comment.
Maybe more on a lower level test but it would be good to also verify the case where the id does not exist in the specified slice but does exist in another slice. I.e., that the implementation does not only prefer the specified slice, it filters out all other slices.
There was a problem hiding this comment.
Added a test ("An id present only in another slice is filtered out"): the id is indexed in slice a only, then GET/delete/update from slice b all report missing and a search scoped to b returns 0 hits — confirming other slices are filtered out, not just de-prioritized.
| - match: { result: deleted } | ||
|
|
||
| - do: | ||
| catch: missing |
There was a problem hiding this comment.
It would be good to also do this with delete and OCC/update. We want it to report "missing/404" and not potentially a "wrong slice" exception.
There was a problem hiding this comment.
Added — the now-missing (slice a, id 1) is exercised with both delete and update, each asserting missing/404 rather than a wrong-slice error.
| .source("field1", "va") | ||
| .routing("s1") | ||
| .setRoutingFromSlice(true) | ||
| .setRefreshPolicy(RefreshPolicy.NONE) |
There was a problem hiding this comment.
Why specify the default refresh policy?
There was a problem hiding this comment.
No reason — it was the default. Removed it.
| assertThat(indexResponse.getItems()[0].getId(), equalTo("1")); | ||
| assertThat(indexResponse.getItems()[0].getResponse().getResult(), equalTo(DocWriteResponse.Result.CREATED)); | ||
| assertThat(indexResponse.getItems()[1].getResponse().getResult(), equalTo(DocWriteResponse.Result.CREATED)); | ||
| refresh("bulk"); |
There was a problem hiding this comment.
I do not see it done here? I would expect:
| refresh("bulk"); | |
| if (randomBoolean()) { | |
| refresh("bulk"); | |
| } |
| XContentHelper.xContentType(index.source()), | ||
| index.routing() | ||
| ) | ||
| new SourceToParse(indexId, index.source(), XContentHelper.xContentType(index.source()), index.routing()) |
There was a problem hiding this comment.
It seems odd that we use the routing value here but uid.slice() below. I wonder if we should use uid.slice() here but also assert that they are the same for slice enabled?
There was a problem hiding this comment.
Done — for a slice-enabled index the replay routing is now taken from uid.slice(), with an assertion that it matches the replayed index.routing(); non-slice indices keep index.routing().
Resolve the compound _id term into its slice on the INDEX replay path and thread it through as routing, matching the DELETE path, and assert the replayed routing agrees with the term. Drop the redundant NONE refresh policies from the translog get test.
Surface the slice top-level as the _slice metadata field (like _routing) rather than under fields, and name the request parameter `slice` and the field `_slice`, matching routing/_routing exactly: no underscore for request inputs, underscore for fields and output.
Describe `slice` as the slice identifier for a slice-enabled index rather than a replacement for routing, and correct the full-restart test doc: a graceful restart flushes on close and recovers from the Lucene commit rather than replaying the translog.
Cover the recovery paths for slice-enabled indices: ops-based and file-based peer recovery, shard relocation, and establishing a new replica while indexing is outstanding. Each asserts that a delete hit only its slice compound term, leaving the same id in another slice intact on the recovered copy.
Slice indexing: unique _id per (id, slice) pair with compound identity storage
For slice-enabled indices (index.slice.enabled=true), the same _id across different _slice values should be treated as independent documents; A write to _id=1, _slice=A must not overwrite _id=1, _slice=B.
Each document indexes two _id terms: a slice-free search term (encodeId(id + "#")) that drives _id search and GET (slice-context-free), and a compound identity term (encodeId(id + "#" + slice)) that scopes uniqueness, versioning, and deletes to the (id, slice) pair. Because # is not in the numeric or base64url alphabet, encodeId always takes the UTF-8 path, making the encoding unambiguous. The user-visible _id remains the plain id; a single helper encodeIdentity(...) is the one place that builds the compound term.
Previously, live docs stored the plain id while tombstones stored the compound bytes, forcing live-vs-tombstone branching across the version-map restore, recovery snapshots, and translog replay. This PR stores compound bytes uniformly for both, mirroring TSDB's synthetic-id model. Slice-awareness now collapses to two points: the mapper write and the IdLoader decode at the presentation layer.
Of course, most of this is tests for ops-based recovery, version-map restore, and scroll slice partitioning.