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

Sync settings #994

Closed
Closed

Conversation

monusingh-1
Copy link
Collaborator

@monusingh-1 monusingh-1 commented Jun 14, 2023

Description

Sync settings when trying to update leader index metadata.

Issues Resolved

#992

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

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: monusingh-1 <[email protected]>
@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #994 (c6f4bef) into main (93205a1) will increase coverage by 24.50%.
The diff coverage is 54.79%.

❗ Current head c6f4bef differs from pull request most recent head b3a3e28. Consider uploading reports for the commit b3a3e28 to get more accurate results

@@              Coverage Diff              @@
##               main     #994       +/-   ##
=============================================
+ Coverage     46.05%   70.55%   +24.50%     
- Complexity      633     1012      +379     
=============================================
  Files           141      141               
  Lines          4736     4809       +73     
  Branches        534      550       +16     
=============================================
+ Hits           2181     3393     +1212     
+ Misses         2291     1068     -1223     
- Partials        264      348       +84     
Impacted Files Coverage Δ
...tion/action/replay/TransportReplayChangesAction.kt 70.00% <54.79%> (+1.03%) ⬆️

... and 72 files with indirect coverage changes

@monusingh-1 monusingh-1 marked this pull request as ready for review June 14, 2023 09:18
@@ -26,14 +26,20 @@ import kotlinx.coroutines.asCoroutineDispatcher
import kotlinx.coroutines.launch
import org.apache.logging.log4j.LogManager
import org.opensearch.action.ActionListener
import org.opensearch.action.admin.indices.alias.IndicesAliasesRequest
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove unused imports in next commit.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Signed-off-by: monusingh-1 <[email protected]>
@@ -166,6 +178,120 @@ class TransportReplayChangesAction @Inject constructor(settings: Settings, trans
return WriteReplicaResult(request, location, null, replicaShard, log)
}

// fetches the index settings from the leader cluster and applies it to the local cluster's index settings.
private suspend fun syncRemoteSettings(leaderAlias: String, leaderIndex: String, followerIndex: String) {
Copy link
Member

Choose a reason for hiding this comment

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

Like discussed offline, let's use common logic between replay and index replication task for mapping and settings sync.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ack

@@ -166,6 +178,120 @@ class TransportReplayChangesAction @Inject constructor(settings: Settings, trans
return WriteReplicaResult(request, location, null, replicaShard, log)
}

// fetches the index settings from the leader cluster and applies it to the local cluster's index settings.
private suspend fun syncRemoteSettings(leaderAlias: String, leaderIndex: String, followerIndex: String) {
Copy link
Member

Choose a reason for hiding this comment

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

This function is being added mainly to be able to update mappings which use analyzer defined in the settings.
We can reduce the scope of this function to just check if there is any change in the index.analysis.* settings and then update that. This way we can remove most of the logic of creating the settings and closing the index.

@@ -124,6 +131,11 @@ class TransportReplayChangesAction @Inject constructor(settings: Settings, trans
var result = primaryShard.applyTranslogOperation(op, Engine.Operation.Origin.PRIMARY)
if (shouldSyncMappingAndRetry(result)) {
waitForMappingUpdate {
// fetch index settings from the leader cluster when applying on PRIMARY...
try{
Copy link
Member

Choose a reason for hiding this comment

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

We should not catch the exception

@monusingh-1 monusingh-1 marked this pull request as draft June 15, 2023 05:04
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