Enforce 512 byte document ID limit in bulk updates #8039

ankitkala · 2023-06-13T10:19:01Z

Description

Enforce 512 byte document ID limit in bulk updates

Related Issues

Resolves #6595

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Ankit Kala <[email protected]>

github-actions · 2023-06-13T10:51:42Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchTaskCancellationWithHighCpu
      1 org.opensearch.cluster.allocation.ClusterRerouteIT.testDelayWithALargeAmountOfShards

URL: https://build.ci.opensearch.org/job/gradle-check/17436/
CommitID: 76abd64
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov · 2023-06-13T10:54:27Z

Codecov Report

Merging #8039 (e4664e8) into main (bb1f7e2) will decrease coverage by 0.21%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #8039      +/-   ##
============================================
- Coverage     71.00%   70.80%   -0.21%     
+ Complexity    56672    56546     -126     
============================================
  Files          4722     4722              
  Lines        267576   267580       +4     
  Branches      39213    39213              
============================================
- Hits         189995   189455     -540     
- Misses        61593    62144     +551     
+ Partials      15988    15981       -7

Impacted Files	Coverage Δ
...in/java/org/opensearch/action/DocWriteRequest.java	`49.31% <100.00%> (+3.72%)`	⬆️
...java/org/opensearch/action/index/IndexRequest.java	`81.90% <100.00%> (+0.77%)`	⬆️
...va/org/opensearch/action/update/UpdateRequest.java	`46.89% <100.00%> (+1.10%)`	⬆️

... and 458 files with indirect coverage changes

github-actions · 2023-06-13T11:12:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchShardTaskCancellationWithHighCpu

URL: https://build.ci.opensearch.org/job/gradle-check/17437/
CommitID: 7b48473
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

andrross

@dblock @reta We need to make a call here as to the backward compatibility concern. This is (IMO) clearly a bug, but it is a breaking change and will break any workloads that knowingly or not rely on being able to use large IDs in update requests. I'm inclined to backport it because it is a bug and any workloads that break should be aware of their reliance on something that was never intended to be supported.

server/src/main/java/org/opensearch/action/DocWriteRequest.java

reta · 2023-06-13T17:19:18Z

@dblock @reta We need to make a call here as to the backward compatibility concern. This is (IMO) clearly a bug, but it is a breaking change and will break any workloads that knowingly or not rely on being able to use large IDs in update requests. I'm inclined to backport it because it is a bug and any workloads that break should be aware of their reliance on something that was never intended to be supported.

@andrross I tend to agree with you here, it seems like there is an implicit limit (which now becomes explicit), so backporting it would make sense. Also, would be great to update documentation (https://github.com/opensearch-project/documentation-website/)

server/src/main/java/org/opensearch/action/DocWriteRequest.java

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

CHANGELOG.md

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

Signed-off-by: Ankit Kala <[email protected]>

ankitkala · 2023-06-14T12:29:27Z

@dblock @reta We need to make a call here as to the backward compatibility concern. This is (IMO) clearly a bug, but it is a breaking change and will break any workloads that knowingly or not rely on being able to use large IDs in update requests. I'm inclined to backport it because it is a bug and any workloads that break should be aware of their reliance on something that was never intended to be supported.

@andrross I tend to agree with you here, it seems like there is an implicit limit (which now becomes explicit), so backporting it would make sense. Also, would be great to update documentation (https://github.com/opensearch-project/documentation-website/)

I'll open an issue to update the documentation after the PR has been merged.

github-actions · 2023-06-14T12:58:08Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testDropPrimaryDuringReplication
      1 org.opensearch.remotestore.RemoteStoreRefreshListenerIT.testRemoteRefreshRetryOnFailure

URL: https://build.ci.opensearch.org/job/gradle-check/17551/
CommitID: ea41546
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

server/src/main/java/org/opensearch/action/DocWriteRequest.java

Signed-off-by: Ankit Kala <[email protected]>

github-actions · 2023-06-14T16:59:28Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.remotestore.RemoteStoreIT.testPeerRecoveryWithRemoteStoreAndRemoteTranslogRefresh
      1 org.opensearch.index.ShardIndexingPressureIT.testShardIndexingPressureTrackingDuringBulkWrites

URL: https://build.ci.opensearch.org/job/gradle-check/17577/
CommitID: 642699b
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

CHANGELOG.md

Signed-off-by: Andrew Ross <[email protected]>

github-actions · 2023-06-14T20:19:13Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/17601/
CommitID: ef60db4
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-06-14T21:58:14Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/17619/
CommitID: ef60db4
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

andrross · 2023-06-15T03:57:09Z

I merged in the latest from main, which should fix the test failures. This is ready to go assuming the check passes.

github-actions · 2023-06-15T04:29:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

URL: https://build.ci.opensearch.org/job/gradle-check/17638/
CommitID: e4664e8
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

* Enforce doc id limit on UpdateRequests as well Signed-off-by: Ankit Kala <[email protected]> * PR comments Signed-off-by: Ankit Kala <[email protected]> * Address comments 2 Signed-off-by: Ankit Kala <[email protected]> * Move changelog entry Signed-off-by: Andrew Ross <[email protected]> --------- Signed-off-by: Ankit Kala <[email protected]> Signed-off-by: Andrew Ross <[email protected]> Co-authored-by: Andrew Ross <[email protected]> (cherry picked from commit 60afeb7) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Enforce doc id limit on UpdateRequests as well * PR comments * Address comments 2 * Move changelog entry --------- (cherry picked from commit 60afeb7) Signed-off-by: Ankit Kala <[email protected]> Signed-off-by: Andrew Ross <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Andrew Ross <[email protected]>

…t#8039) (opensearch-project#8081) * Enforce doc id limit on UpdateRequests as well * PR comments * Address comments 2 * Move changelog entry --------- (cherry picked from commit 60afeb7) Signed-off-by: Ankit Kala <[email protected]> Signed-off-by: Andrew Ross <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Andrew Ross <[email protected]>

…t#8039) * Enforce doc id limit on UpdateRequests as well Signed-off-by: Ankit Kala <[email protected]> * PR comments Signed-off-by: Ankit Kala <[email protected]> * Address comments 2 Signed-off-by: Ankit Kala <[email protected]> * Move changelog entry Signed-off-by: Andrew Ross <[email protected]> --------- Signed-off-by: Ankit Kala <[email protected]> Signed-off-by: Andrew Ross <[email protected]> Co-authored-by: Andrew Ross <[email protected]> Signed-off-by: Rishab Nahata <[email protected]>

…t#8039) * Enforce doc id limit on UpdateRequests as well Signed-off-by: Ankit Kala <[email protected]> * PR comments Signed-off-by: Ankit Kala <[email protected]> * Address comments 2 Signed-off-by: Ankit Kala <[email protected]> * Move changelog entry Signed-off-by: Andrew Ross <[email protected]> --------- Signed-off-by: Ankit Kala <[email protected]> Signed-off-by: Andrew Ross <[email protected]> Co-authored-by: Andrew Ross <[email protected]> Signed-off-by: Shivansh Arora <[email protected]>

Enforce doc id limit on UpdateRequests as well

7b48473

Signed-off-by: Ankit Kala <[email protected]>

ankitkala force-pushed the bulk branch from 76abd64 to 7b48473 Compare June 13, 2023 10:38

ankitkala marked this pull request as ready for review June 13, 2023 11:25

ankitkala mentioned this pull request Jun 13, 2023

[BUG] Indexing a document through the bulk api bypasses the 512 byte _id size limit #6595

Closed

andrross reviewed Jun 13, 2023

View reviewed changes

server/src/main/java/org/opensearch/action/DocWriteRequest.java Outdated Show resolved Hide resolved

reta reviewed Jun 13, 2023

View reviewed changes

server/src/main/java/org/opensearch/action/DocWriteRequest.java Outdated Show resolved Hide resolved

psychbot reviewed Jun 13, 2023

View reviewed changes

server/src/main/java/org/opensearch/action/DocWriteRequest.java Outdated Show resolved Hide resolved

server/src/internalClusterTest/java/org/opensearch/action/bulk/BulkIntegrationIT.java Show resolved Hide resolved

andrross reviewed Jun 13, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

mch2 reviewed Jun 13, 2023

View reviewed changes

server/src/internalClusterTest/java/org/opensearch/action/bulk/BulkIntegrationIT.java Show resolved Hide resolved

PR comments

ea41546

Signed-off-by: Ankit Kala <[email protected]>

ankitkala changed the title ~~Enforce doc id limit on UpdateRequests as well~~ Enforce 512 byte document ID limit in bulk updates Jun 14, 2023

reta reviewed Jun 14, 2023

View reviewed changes

server/src/main/java/org/opensearch/action/DocWriteRequest.java Outdated Show resolved Hide resolved

Address comments 2

642699b

Signed-off-by: Ankit Kala <[email protected]>

reta approved these changes Jun 14, 2023

View reviewed changes

andrross reviewed Jun 14, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Move changelog entry

ef60db4

Signed-off-by: Andrew Ross <[email protected]>

andrross added the backport 2.x Backport to 2.x branch label Jun 14, 2023

Merge branch 'main' into bulk

e4664e8

sachinpkale approved these changes Jun 15, 2023

View reviewed changes

sachinpkale merged commit 60afeb7 into opensearch-project:main Jun 15, 2023

opensearch-trigger-bot bot mentioned this pull request Jun 15, 2023

[Backport 2.x] Enforce 512 byte document ID limit in bulk updates #8081

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce 512 byte document ID limit in bulk updates #8039

Enforce 512 byte document ID limit in bulk updates #8039

ankitkala commented Jun 13, 2023 •

edited

Loading

github-actions bot commented Jun 13, 2023

codecov bot commented Jun 13, 2023 •

edited

Loading

github-actions bot commented Jun 13, 2023

andrross left a comment

reta commented Jun 13, 2023

ankitkala commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

andrross commented Jun 15, 2023

github-actions bot commented Jun 15, 2023

Enforce 512 byte document ID limit in bulk updates #8039

Enforce 512 byte document ID limit in bulk updates #8039

Conversation

ankitkala commented Jun 13, 2023 • edited Loading

Description

Related Issues

Check List

github-actions bot commented Jun 13, 2023

Gradle Check (Jenkins) Run Completed with:

codecov bot commented Jun 13, 2023 • edited Loading

Codecov Report

github-actions bot commented Jun 13, 2023

Gradle Check (Jenkins) Run Completed with:

andrross left a comment

Choose a reason for hiding this comment

reta commented Jun 13, 2023

ankitkala commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jun 14, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jun 14, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jun 14, 2023

Gradle Check (Jenkins) Run Completed with:

andrross commented Jun 15, 2023

github-actions bot commented Jun 15, 2023

Gradle Check (Jenkins) Run Completed with:

ankitkala commented Jun 13, 2023 •

edited

Loading

codecov bot commented Jun 13, 2023 •

edited

Loading