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

Changed names for assume roles to remote and local from leader and follower #29

Conversation

saikaranam-amazon
Copy link
Member

@saikaranam-amazon saikaranam-amazon commented Jun 30, 2021

Description

  • Changed names for assume roles to remote and local from leader and follower
  • Added preference to fetch the metdata from the primary shard
  • Additional logging for the metadata store

Issues Resolved

N/A

Check List

  • 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.

Comment on lines 130 to 131
log.debug("Prefer node is ${shardRouting.currentNodeId()}")
return "_prefer_nodes:${shardRouting.currentNodeId()}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Should we be using _only_nodes instead or probably have a param which will do that? (Something like DDB consistent reads where the callers can explicitly opt for getting consistent state where required and getting from replica where slight staleness is ok)
  2. Can we include the index or something as well in the log? (Basically trying to see if we can connect the log statement to appropriate index if that isn't part of log context)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the index info to the log. we can discuss regarding the _only_nodes as param

krishna-ggk
krishna-ggk previously approved these changes Jun 30, 2021
Copy link
Collaborator

@krishna-ggk krishna-ggk left a comment

Choose a reason for hiding this comment

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

Please update the discussion details on preference. Rest looks good.

Comment on lines -55 to 57
const val LEADER_FGAC_ROLE = "leader_fgac_role"
const val FOLLOWER_FGAC_ROLE = "follower_fgac_role"
const val LEADER_FGAC_ROLE = "remote_cluster_role"
const val FOLLOWER_FGAC_ROLE = "local_cluster_role"
private val INDEX_REQ_PARSER = ObjectParser<ReplicateIndexRequest, Void>("FollowIndexRequestParser") { ReplicateIndexRequest() }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit - we could have renamed var names as well.

Copy link
Collaborator

@gbbafna gbbafna left a comment

Choose a reason for hiding this comment

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

LGTM. One minor comment.

naveenpajjuri
naveenpajjuri previously approved these changes Jun 30, 2021
@@ -66,7 +66,7 @@ fun <Req, Resp> Client.suspending(fn: (Req, ActionListener<Resp>) -> Unit,
}
}

fun <Req, Resp> Client.suspending(replicationMetadata: ReplicationMetadata,
suspend fun <Req, Resp> Client.suspending(replicationMetadata: ReplicationMetadata,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for my understanding, Why are we making these suspend?
because some non suspend methods might be calling them right?

gbbafna
gbbafna previously approved these changes Jun 30, 2021
…llower

Added preference to fetch the metdata from the primary shard
Additional logging for the metadata store
@saikaranam-amazon saikaranam-amazon merged commit d97d41f into opensearch-project:main Jun 30, 2021
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