Remove seq_no from bulk response when disabled by romseygeek · Pull Request #143753 · elastic/elasticsearch

romseygeek · 2026-03-06T16:15:48Z

Sequence number and primary term are retained on the internal
Engine.Result objects, but removed from REST-layer serialization.

Sequence number and primary term are retained on the internal Engine.Result objects, but removed from REST-layer serialization.

elasticsearchmachine · 2026-03-06T16:16:15Z

Pinging @elastic/es-distributed (Team:Distributed)

fcofdez

LGTM. I left a minor comment that we might want to address.

fcofdez · 2026-03-09T09:13:21Z

server/src/main/java/org/elasticsearch/action/bulk/BulkPrimaryExecutionContext.java

        final BulkItemRequest current = getCurrentItem();
        DocWriteRequest<?> docWriteRequest = getRequestToExecute();
        switch (result.getResultType()) {
            case SUCCESS -> {


Apparently we return a sequence number of failures too (I guess it's just the unassigned seq no), but maybe for completeness we should follow the same logic there and ensure that we return the unassigned seq no?

It's kept internally but the serialized XContent doesn't include it.

…-bulk-response

romseygeek · 2026-03-09T15:53:58Z

I ended up having to move the sequence number redaction into the BulkItemResponse handling, rather than in DocWriteResponse, as that's still needed for internal replication correctness. It's sufficiently different that I think it needs another review.

tlrx

Just to be sure to understand, we remove the seq no/primary term from Bulk responses REST API but we return sentinel values for Searches and GETs? I probably missed a point but this inconsistency looks trappy to me.

tlrx · 2026-03-10T09:29:27Z

server/src/internalClusterTest/java/org/elasticsearch/action/bulk/BulkIntegrationIT.java

+            .setSettings(
+                Settings.builder()
+                    .put(IndexSettings.DISABLE_SEQUENCE_NUMBERS.getKey(), true)
+                    .put(IndexSettings.SEQ_NO_INDEX_OPTIONS_SETTING.getKey(), "doc_values_only")


nit:

Suggested change

.put(IndexSettings.SEQ_NO_INDEX_OPTIONS_SETTING.getKey(), "doc_values_only")

.put(IndexSettings.SEQ_NO_INDEX_OPTIONS_SETTING.getKey(), SeqNoFieldMapper.SeqNoIndexOptions.DOC_VALUES_ONLY)

tlrx · 2026-03-10T09:35:06Z

server/src/main/java/org/elasticsearch/action/ingest/SimulateIndexResponse.java


+    @Override
+    public SimulateIndexResponse withoutSequenceNumber() {
+        throw new UnsupportedOperationException("SimulateIndexResponse does not support withoutSequenceNumber");


Looks like we could support this too since SimulateIndexResponse already redacts seq_no/primary term ?

See comment // We don't actually care about most of the IndexResponse fields:

tlrx · 2026-03-10T09:36:16Z

server/src/test/java/org/elasticsearch/action/bulk/TransportShardBulkActionTests.java

+        assumeTrue("Test should only run with feature flag", IndexSettings.DISABLE_SEQUENCE_NUMBERS_FEATURE_FLAG);
+        Settings settings = Settings.builder()
+            .put(IndexSettings.DISABLE_SEQUENCE_NUMBERS.getKey(), true)
+            .put(IndexSettings.SEQ_NO_INDEX_OPTIONS_SETTING.getKey(), "doc_values_only")


nit:

Suggested change

.put(IndexSettings.SEQ_NO_INDEX_OPTIONS_SETTING.getKey(), "doc_values_only")

.put(IndexSettings.SEQ_NO_INDEX_OPTIONS_SETTING.getKey(), SeqNoFieldMapper.SeqNoIndexOptions.DOC_VALUES_ONLY)

fcofdez · 2026-03-10T09:53:47Z

Just to be sure to understand, we remove the seq no/primary term from Bulk responses REST API but we return sentinel values for Searches and GETs? I probably missed a point but this inconsistency looks trappy to me.

It looks like it's the current behaviour (see

elasticsearch/server/src/main/java/org/elasticsearch/action/DocWriteResponse.java

Lines 284 to 287 in 3762635

    
           if (getSeqNo() >= 0) { 
        
               builder.field(_SEQ_NO, getSeqNo()); 
        
               builder.field(_PRIMARY_TERM, getPrimaryTerm()); 
        
           }

). But I agree that this inconsistency can be trappy and I wonder if we could break clients with this change. I think that we should return back the sentinel values indeed.

fcofdez · 2026-03-10T09:55:01Z

server/src/main/java/org/elasticsearch/action/bulk/BulkOperation.java


    private void completeBulkOperation() {
+        BulkItemResponse[] bulkItemResponses = responses.toArray(new BulkItemResponse[responses.length()]);
+        redactSeqNoFromDisabledIndices(bulkItemResponses);


We iterate over the responses once when a shard responds back (see

elasticsearch/server/src/main/java/org/elasticsearch/action/bulk/BulkOperation.java

Line 501 in d3e0128

BulkItemResponse bulkItemResponse = bulkShardResponse.getResponses()[idx];

) Maybe we should do this transformation there instead?

romseygeek · 2026-03-10T11:32:05Z

I think that we should return back the sentinel values indeed.

I can certainly update things to do that. Do you think we should also change it for existing indexes, or just for bulk loads with sequence numbers disabled?

romseygeek · 2026-03-10T11:41:00Z

I can certainly update things to do that. Do you think we should also change it for existing indexes, or just for bulk loads with sequence numbers disabled?

Actually, looking again, I think we will have to always return sentinel values if the sequence number isn't assigned. But that should be ok, as the XContent serialization is only used at the REST layer so existing workflows will always have sequence numbers set and the current suppression of values won't have an effect outside of tests.

…-bulk-response

…primary_term

…esponse' into seqno/remove-seq-from-bulk-response

fcofdez

Looks good! Thanks for the iterations, I left a few suggestions before approving this.

fcofdez · 2026-03-10T12:38:30Z

server/src/main/java/org/elasticsearch/action/DocWriteResponse.java

-            builder.field(_SEQ_NO, getSeqNo());
-            builder.field(_PRIMARY_TERM, getPrimaryTerm());
-        }
+        builder.field(_SEQ_NO, getSeqNo());


My guess is that this was here for legacy reasons (to support responses before we introduced sequence numbers) so I guess that it's ok to remove the check.

fcofdez · 2026-03-10T13:29:27Z

server/src/main/java/org/elasticsearch/action/update/UpdateResponse.java

+            getVersion(),
+            result
+        );
+        copy.setGetResult(getResult);


don't we need to copy the shard info here?

It's using the constructor that takes ShardInfo so I think we're good

fcofdez · 2026-03-10T13:29:49Z

server/src/main/java/org/elasticsearch/action/bulk/BulkPrimaryExecutionContext.java

                        primary.shardId(),
                        requestToExecute.id(),
-                        deleteResult.getSeqNo(),
+                        result.getSeqNo(),


I think that we can revert this?

fcofdez · 2026-03-10T13:34:01Z

server/src/main/java/org/elasticsearch/action/bulk/BulkOperation.java

                }

+                private BulkItemResponse maybeRedactSequenceNumber(BulkItemResponse in) {
+                    if (IndexSettings.DISABLE_SEQUENCE_NUMBERS_FEATURE_FLAG == false) {


We grab the index metadata in

elasticsearch/server/src/main/java/org/elasticsearch/action/bulk/BulkOperation.java

Line 405 in d3e0128

var indexMetadata = project.getIndexSafe(shardId.getIndex());

maybe we can pass the boolean flag into BulkOperation#executeBulkShardRequest directly so we don't need to go through the project metadata per response?

I hadn't appreciated that everything would be from a single index, but of course this is per-shard isn't it. Have updated.

fcofdez

LGTM. Thanks for the iterations!

Remove seq_no from bulk response when disabled

132dcf4

Sequence number and primary term are retained on the internal Engine.Result objects, but removed from REST-layer serialization.

romseygeek requested review from fcofdez, kkrik-es and tlrx March 6, 2026 16:15

romseygeek self-assigned this Mar 6, 2026

romseygeek added >non-issue :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. v9.4.0 labels Mar 6, 2026

elasticsearchmachine added the Team:Distributed Meta label for distributed team. label Mar 6, 2026

romseygeek mentioned this pull request Mar 6, 2026

Trim _seq_no doc values when no longer needed #136305

Closed

19 tasks

kkrik-es approved these changes Mar 6, 2026

View reviewed changes

test

d8d189b

fcofdez approved these changes Mar 9, 2026

View reviewed changes

romseygeek added 3 commits March 9, 2026 09:26

Merge remote-tracking branch 'origin/main' into seqno/remove-seq-from…

9b8ce9e

…-bulk-response

Move seq_no redaction to serialization point

deeb9ee

Merge remote-tracking branch 'origin/main' into seqno/remove-seq-from…

3855102

…-bulk-response

romseygeek requested a review from fcofdez March 9, 2026 15:54

tlrx reviewed Mar 10, 2026

View reviewed changes

fcofdez reviewed Mar 10, 2026

View reviewed changes

romseygeek and others added 5 commits March 10, 2026 11:47

Merge remote-tracking branch 'origin/main' into seqno/remove-seq-from…

ac65e77

…-bulk-response

rework BulkOperation to a single loop; always serialize _seq_no and _…

8972d04

…primary_term

[CI] Auto commit changes from spotless

d7981f7

tests: update no longer supported without seq no

17f511b

Merge remote-tracking branch 'romseygeek/seqno/remove-seq-from-bulk-r…

e23db8d

…esponse' into seqno/remove-seq-from-bulk-response

fcofdez reviewed Mar 10, 2026

View reviewed changes

moar

0b0504f

fcofdez approved these changes Mar 10, 2026

View reviewed changes

romseygeek merged commit 5a6352b into elastic:main Mar 10, 2026
36 checks passed

	.put(IndexSettings.SEQ_NO_INDEX_OPTIONS_SETTING.getKey(), "doc_values_only")
	.put(IndexSettings.SEQ_NO_INDEX_OPTIONS_SETTING.getKey(), SeqNoFieldMapper.SeqNoIndexOptions.DOC_VALUES_ONLY)

Conversation

romseygeek commented Mar 6, 2026

Uh oh!

elasticsearchmachine commented Mar 6, 2026

Uh oh!

fcofdez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Mar 9, 2026

Uh oh!

tlrx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcofdez commented Mar 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romseygeek commented Mar 10, 2026

Uh oh!

romseygeek commented Mar 10, 2026

Uh oh!

fcofdez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcofdez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants