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

[Segment Replication] Refactor Store.recoveryDiff method #4221

Closed

Conversation

dreamer-89
Copy link
Member

Signed-off-by: Suraj Singh surajrider@gmail.com

Description

This is a cleanup change, coming from https://github.com/opensearch-project/OpenSearch/pull/4185/files#r944055590. This PR refactors Store.recoveryDiff to make it usable for segment replication and removes previously added methods.

Issues Resolved

#3787

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.

@dreamer-89 dreamer-89 requested review from a team and reta as code owners August 16, 2022 02:17
@dreamer-89 dreamer-89 requested a review from kartg August 16, 2022 02:17
@github-actions

This comment was marked as outdated.

@dreamer-89
Copy link
Member Author

Surprisingly same failure in another unrelated #4222. Refiring

REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.index.fielddata.SortedSetDVStringFieldDataTests.testSortMissingFirstReverse" -Dtests.seed=3D8AA4A9AB9F1767 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=el-CY -Dtests.timezone=Africa/Maputo -Druntime.java=17

org.opensearch.index.fielddata.SortedSetDVStringFieldDataTests > testSortMissingFirstReverse FAILED

@github-actions

This comment was marked as outdated.

@kartg
Copy link
Member

kartg commented Aug 16, 2022

Looks like a flaky test failure with mixedClusterTest

REPRODUCE WITH: ./gradlew ':qa:mixed-cluster:v2.3.0#mixedClusterTest' --tests "org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=search/190_index_prefix_search/search with uppercase regex}" -Dtests.seed=3027526E1233881C -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=en-CA -Dtests.timezone=Australia/Brisbane -Druntime.java=17

org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT > test {p0=search/190_index_prefix_search/search with uppercase regex} FAILED
    java.lang.AssertionError: Failure at [search/190_index_prefix_search:97]: hits.total didn't match expected value:
                        hits.total: expected Integer [1] but was Integer [0]

Will refire gradle check

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@dreamer-89
Copy link
Member Author

Last gradle check failed with legitimate failure. The test failed because for SegmentReplication need to pass false for enforceGroupConsistency boolean param. Updated. Refiring!

REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.index.shard.SegmentReplicationIndexShardTests.testSegmentReplication_Index_Update_Delete" -Dtests.seed=BAE79CF96613AC1E -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=en-AU -Dtests.timezone=Etc/GMT-11 -Druntime.java=17

org.opensearch.index.shard.SegmentReplicationIndexShardTests > testSegmentReplication_Index_Update_Delete FAILED
    java.lang.AssertionError: test replication should not fail: OpenSearchException[Segment Replication failed]; nested: IllegalStateException[Shard [test][0] has local copies of segments that differ from the primary];
        at __randomizedtesting.SeedInfo.seed([BAE79CF96613AC1E:26AB30DC8C7FE0C5]:0)

@dreamer-89
Copy link
Member Author

Pre-commit failing with below error; probably due to recent dependabot version bump of gradle enterprise.

* Where:
Settings file '/home/runner/work/OpenSearch/OpenSearch/settings.gradle' line: 13

* What went wrong:
Plugin [id: 'com.gradle.enterprise', version: '3.11.1'] was not found in any of the following sources:

@github-actions

This comment was marked as outdated.

@dreamer-89
Copy link
Member Author

dreamer-89 commented Aug 23, 2022

gradle check failing with 530 server error. Probably need to retry after some time. This version was introduced in recent dependabot PR #4273

FAILURE: Build failed with an exception.

* What went wrong:
Could not resolve all files for configuration 'classpath'.
> Could not resolve com.gradle:gradle-enterprise-gradle-plugin:3.11.1.
  Required by:
      unspecified:unspecified:unspecified > com.gradle.enterprise:com.gradle.enterprise.gradle.plugin:3.11.1
   > Could not resolve com.gradle:gradle-enterprise-gradle-plugin:3.11.1.
      > Could not get resource 'https://plugins.gradle.org/m2/com/gradle/gradle-enterprise-gradle-plugin/3.11.1/gradle-enterprise-gradle-plugin-3.11.1.pom'.
         > Could not GET 'https://plugins.gradle.org/m2/com/gradle/gradle-enterprise-gradle-plugin/3.11.1/gradle-enterprise-gradle-plugin-3.11.1.pom'. Received status code 530 from server: 

@github-actions

This comment was marked as outdated.

Signed-off-by: Suraj Singh <surajrider@gmail.com>
Signed-off-by: Suraj Singh <surajrider@gmail.com>
Signed-off-by: Suraj Singh <surajrider@gmail.com>
Signed-off-by: Suraj Singh <surajrider@gmail.com>
@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2022

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

Codecov Report

Merging #4221 (48523ea) into main (70d911c) will decrease coverage by 0.45%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #4221      +/-   ##
============================================
- Coverage     71.08%   70.62%   -0.46%     
+ Complexity    57622    57258     -364     
============================================
  Files          4616     4616              
  Lines        275441   275416      -25     
  Branches      40314    40310       -4     
============================================
- Hits         195801   194525    -1276     
- Misses        63602    64591     +989     
- Partials      16038    16300     +262     
Impacted Files Coverage Δ
...rc/main/java/org/opensearch/index/store/Store.java 81.20% <100.00%> (-0.98%) ⬇️
.../indices/replication/SegmentReplicationTarget.java 90.99% <100.00%> (ø)
...g/opensearch/index/analysis/CharFilterFactory.java 0.00% <0.00%> (-100.00%) ⬇️
...ava/org/opensearch/action/NoSuchNodeException.java 0.00% <0.00%> (-50.00%) ⬇️
...opensearch/persistent/PersistentTasksExecutor.java 22.22% <0.00%> (-44.45%) ⬇️
...search/aggregations/pipeline/HoltWintersModel.java 21.47% <0.00%> (-43.63%) ⬇️
.../admin/cluster/reroute/ClusterRerouteResponse.java 60.00% <0.00%> (-40.00%) ⬇️
...luster/routing/allocation/RoutingExplanations.java 62.06% <0.00%> (-37.94%) ⬇️
...cluster/coordination/PublishClusterStateStats.java 33.33% <0.00%> (-37.51%) ⬇️
...java/org/opensearch/threadpool/ThreadPoolInfo.java 56.25% <0.00%> (-37.50%) ⬇️
... and 464 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@dreamer-89
Copy link
Member Author

With #4366, which changes the params in Store.recoverDiff for Segment Replication, this PR changes are not required anymore. Converting this to first draft to hear back in case of any concerns and will finally close it.

CC @mch2 @kartg

@dreamer-89 dreamer-89 marked this pull request as draft September 6, 2022 15:36
@dreamer-89
Copy link
Member Author

dreamer-89 commented Sep 9, 2022

Closing this PR as it is not needed anymore because of recent changes in the way difference of snapshots is evaluated.

PR: #4366

CC @mch2 @kartg @anasalkouz

@dreamer-89 dreamer-89 closed this Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants