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.

Sign up for GitHub

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

Jump to bottom

Support for translog fetch at the leader(remote) cluster #25

Merged
merged 1 commit into from
Jul 1, 2021
Merged

Support for translog fetch at the leader(remote) cluster #25

merged 1 commit into from
Jul 1, 2021

Conversation

saikaranam-amazon
Copy link
Member

@saikaranam-amazon saikaranam-amazon commented Jun 28, 2021 •
edited
Loading

Description

  • Support for Translog fetch at the leader(remote) cluster
  • and enable Translog pruning during replication setup

Issues Resolved

N/A

Testing

  • Tested locally.
curl "{leader}/remote-index/_settings?pretty"
{
  "remote-index" : {
    "settings" : {
      "index" : {
        "number_of_shards" : "1",
        "translog" : {
          "retention_lease" : {
            "pruning" : {
              "enabled" : "true"
            }
          }
        },
        "provided_name" : "remote-index",
        "creation_date" : "1624936380777",
        "number_of_replicas" : "1",
        "uuid" : "UnLK64RiQpW3kAHOxkZNsQ",
        "version" : {
          "created" : "7100299"
        }
      }
    }
  }
}

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.

@saikaranam-amazon saikaranam-amazon force-pushed the fetch-translog branch 2 times, most recently from 7235098 to da704a6 Compare June 29, 2021 03:21
krishna-ggk
krishna-ggk reviewed Jun 29, 2021
View reviewed changes
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.

Thanks for the changes and looks good overall.

Would be super awesome to include some tests - anything that stops us from that?

...main/kotlin/com/amazon/elasticsearch/replication/action/changes/TransportGetChangesAction.kt Outdated Show resolved Hide resolved
...in/kotlin/com/amazon/elasticsearch/replication/action/replay/TransportReplayChangesAction.kt Outdated Show resolved Hide resolved
...in/kotlin/com/amazon/elasticsearch/replication/action/replay/TransportReplayChangesAction.kt Outdated Show resolved Hide resolved
src/main/kotlin/com/amazon/elasticsearch/replication/seqno/RemoteClusterTranslogService.kt Outdated Show resolved Hide resolved
src/main/kotlin/com/amazon/elasticsearch/replication/seqno/RemoteClusterTranslogService.kt Outdated Show resolved Hide resolved
src/main/kotlin/com/amazon/elasticsearch/replication/seqno/RemoteClusterTranslogService.kt Outdated Show resolved Hide resolved
src/main/kotlin/com/amazon/elasticsearch/replication/seqno/RemoteClusterTranslogService.kt Outdated Show resolved Hide resolved
src/main/kotlin/com/amazon/elasticsearch/replication/seqno/RemoteClusterTranslogService.kt Outdated Show resolved Hide resolved
@saikaranam-amazon saikaranam-amazon force-pushed the fetch-translog branch 2 times, most recently from 596e8f0 to c5b2e31 Compare June 29, 2021 12:02
@saikaranam-amazon saikaranam-amazon force-pushed the fetch-translog branch 2 times, most recently from d5e3af4 to ff7fde4 Compare June 30, 2021 05:01
naveenpajjuri
naveenpajjuri previously approved these changes Jun 30, 2021
View reviewed changes
Copy link
Contributor

@naveenpajjuri naveenpajjuri left a comment

Choose a reason for hiding this comment

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

LGTM

krishna-ggk
krishna-ggk reviewed Jun 30, 2021
View reviewed changes
...main/kotlin/com/amazon/elasticsearch/replication/action/changes/TransportGetChangesAction.kt Outdated
Comment on lines 94 to 103
var ops: List<Translog.Operation> = listOf()
var fetchFromLucene = !isTranslogPruningByRetentionLeaseEnabled(shardId)
try {
ops = translogService.getHistoryOfOperations(indexShard, request.fromSeqNo, toSeqNo)
} catch (e: ResourceNotFoundException) {
fetchFromLucene = true
}

if(fetchFromLucene) {
log.debug("Fetching changes from lucene for ${request.shardId} - from:${request.fromSeqNo}, to:$toSeqNo")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The initial fetchFromLucene is misleading as we are fetching from translog first.

Was the intent to fetch from lucence if translog setting is absent?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As we discussed offline, we make the log info for now so we can get some initial input from test runs as to how often we fall back to lucene.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

krishna-ggk
krishna-ggk reviewed Jun 30, 2021
View reviewed changes
src/main/kotlin/com/amazon/elasticsearch/replication/seqno/RemoteClusterTranslogService.kt Outdated Show resolved Hide resolved
...main/kotlin/com/amazon/elasticsearch/replication/action/changes/TransportGetChangesAction.kt Outdated
Comment on lines 94 to 103
var ops: List<Translog.Operation> = listOf()
var fetchFromLucene = !isTranslogPruningByRetentionLeaseEnabled(shardId)
try {
ops = translogService.getHistoryOfOperations(indexShard, request.fromSeqNo, toSeqNo)
} catch (e: ResourceNotFoundException) {
fetchFromLucene = true
}

if(fetchFromLucene) {
log.debug("Fetching changes from lucene for ${request.shardId} - from:${request.fromSeqNo}, to:$toSeqNo")
Copy link
Collaborator

Choose a reason for hiding this comment

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

As we discussed offline, we make the log info for now so we can get some initial input from test runs as to how often we fall back to lucene.

krishna-ggk
krishna-ggk previously approved these changes Jun 30, 2021
View reviewed changes
krishna-ggk
krishna-ggk previously approved these changes Jun 30, 2021
View reviewed changes
@saikaranam-amazon
Support for translog fetch at the leader(remote) cluster
145ec1c 
and enable translog pruning during replication setup
naveenpajjuri
naveenpajjuri approved these changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@naveenpajjuri naveenpajjuri left a comment

Choose a reason for hiding this comment

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

LGTM

@krishna-ggk krishna-ggk merged commit f04c3b1 into opensearch-project:main Jul 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@naveenpajjuri naveenpajjuri naveenpajjuri approved these changes

@krishna-ggk krishna-ggk krishna-ggk approved these changes

Assignees

@krishna-ggk krishna-ggk

@naveenpajjuri naveenpajjuri

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@saikaranam-amazon @krishna-ggk @naveenpajjuri