Skip to content
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

[BUG] Bug fix for checksum validation for mapping metadata #15885

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

himshikha
Copy link
Contributor

Description

During checksum validation for remote cluster state, for mapping metadata we need to ignore compressed data check.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

Copy link
Contributor

✅ Gradle check result for 31f29c2: SUCCESS

Copy link

codecov bot commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.91%. Comparing base (7cb2bd0) to head (174d10d).
Report is 21 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #15885      +/-   ##
============================================
+ Coverage     71.85%   71.91%   +0.06%     
- Complexity    64213    64235      +22     
============================================
  Files          5272     5272              
  Lines        300538   300578      +40     
  Branches      43432    43432              
============================================
+ Hits         215947   216160     +213     
+ Misses        66833    66639     -194     
- Partials      17758    17779      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@Bukhtawar Bukhtawar left a comment

Choose a reason for hiding this comment

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

LGTM, needs more tests?

@himshikha
Copy link
Contributor Author

LGTM, needs more tests?

Code coverage is failing for the toString method added. Do we really need to add test?

Signed-off-by: Himshikha Gupta <[email protected]>
Copy link
Contributor

✅ Gradle check result for 174d10d: SUCCESS

@Bukhtawar Bukhtawar merged commit f8515c7 into opensearch-project:main Sep 10, 2024
34 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 10, 2024
* Bug fix for checksum validation for mapping metadata

Signed-off-by: Himshikha Gupta <[email protected]>
(cherry picked from commit f8515c7)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 10, 2024
* Bug fix for checksum validation for mapping metadata

Signed-off-by: Himshikha Gupta <[email protected]>
(cherry picked from commit f8515c7)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
himshikha added a commit to himshikha/OpenSearch that referenced this pull request Sep 10, 2024
…h-project#15885)

* Bug fix for checksum validation for mapping metadata

Signed-off-by: Himshikha Gupta <[email protected]>
@@ -169,6 +170,10 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeByteArray(bytes);
}

public void writeVerifiableTo(BufferedChecksumStreamOutput out) throws IOException {
out.writeInt(crc32);
Copy link
Member

Choose a reason for hiding this comment

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

I see that we are ignoring bytes field since that can be different. In that case, should we ingore crc32 as well since this is the checksum and this will be different as well if the bytes are different.

Bukhtawar pushed a commit that referenced this pull request Sep 12, 2024
…adata #15888  (#15890)

* [BUG] Bug fix for checksum validation for mapping metadata (#15885)

* Bug fix for checksum validation for mapping metadata

Signed-off-by: Himshikha Gupta <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 12, 2024
…adata #15888  (#15890)

* [BUG] Bug fix for checksum validation for mapping metadata (#15885)

* Bug fix for checksum validation for mapping metadata

Signed-off-by: Himshikha Gupta <[email protected]>
(cherry picked from commit 1074c71)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Bukhtawar pushed a commit that referenced this pull request Sep 12, 2024
…adata #15888  (#15890) (#15908)

* [BUG] Bug fix for checksum validation for mapping metadata (#15885)

Signed-off-by: Himshikha Gupta <[email protected]>
@reta reta added v2.18.0 Issues and PRs related to version 2.18.0 v2.17.1 Issues and PRs related to version 2.17.1 and removed v2.17.1 Issues and PRs related to version 2.17.1 labels Sep 16, 2024
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 16, 2024
…h-project#15885)

* Bug fix for checksum validation for mapping metadata

Signed-off-by: Himshikha Gupta <[email protected]>
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 17, 2024
…h-project#15885)

* Bug fix for checksum validation for mapping metadata

Signed-off-by: Himshikha Gupta <[email protected]>
dk2k pushed a commit to dk2k/OpenSearch that referenced this pull request Oct 21, 2024
…h-project#15885)

* Bug fix for checksum validation for mapping metadata

Signed-off-by: Himshikha Gupta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport 2.17 skip-changelog v2.18.0 Issues and PRs related to version 2.18.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants