[Segment Replication] Fix timeout issue by calculating time needed to process getSegmentFiles.

[Rishikesh1159]

Signed-off-by: Rishikesh1159 rishireddy1159@gmail.com

Description

This PR fixes timeout issue temporarily for experimental release of segment-replication.

Issues Resolved

#4392

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
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/2748/
• CommitID: 8138702

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/2754/
• CommitID: 3fb0e25

[dreamer-89]

Should this be initialized with 0 ?

[dreamer-89]

I think this initialization can reduced as mentioned in below comment.

[dreamer-89]

These can be reduced with below

final long sizeOfSegmentFiles = filesToFetch.stream().mapToLong(file -> file.length()).sum();

[dreamer-89]

1. Can you share steps performed to identify this baseline ?
2. This baseline is calculated for m5.xlarge instance type, which means all instance types with lower throughput may fail persistently ? Can we relax(reduce) this baseline to smaller value to accomodate lower grade instance types. It doesn't hurt estimated time for high grade instance type, as we estimate higher number of minutes needed but actual transfer will complete faster. Though on the other hand, it will prevent timeout issues on lower grade machines.

[mch2]

+1. This feels a bit high, maybe bump by 1M for every 100mb? This value should be toggleable with a setting, we can shoot for that in next release.

[Rishikesh1159]

Steps to identify baseline:
-> I started a 3 node cluster and ran opensearch-benchmarks with SO dataset. I logged files to fetch size and check the logs later for every replication event. After checking all replication event logs i observed the maximum files to fetch size that has finished within 1min time was 330mb, so I took 330mb as baseline. Later after this change no timeouts happened.

->As I mentioned above that after observing logs I put baseline as 330mb, but as both @dreamer-89 and @mch2 mentioned it makes sense to reduce the baseline, so that lower throughput machines don't have a timeout. I will use bump by 1min for every 100 mb as @mch2 suggested.

[dreamer-89]

Can this be reduced with below ?

final long timeToGetSegmentFiles = Math.max (1, sizeOfSegmentFiles / baseSegmentFilesSize);

[Rishikesh1159]

Let's take example where baseline is 100000000 bytes (100mb) and size of segment files is 250000000(250mb). When we do a sizeOfSegmentFiles / baseSegmentFilesSize result will be 2min. But we need 3min to process this size of segment, so we will have a timeout. To avoid this we need to add a 1 before and after " sizeOfSegmentFiles / baseSegmentFilesSize"

[dreamer-89]

Why this change is needed ?

[Rishikesh1159]

So, while testing benchmarks there are scenarios where the size of files to fetch will be in negative. May be something like this "-1239252992" bytes. If size of files to fetch is negative then time value will also be negative, which doesn't makes sense and throws error. I tried to use Math.abs() but that is forbidden so I have no other option. Please suggest if there are any ways to overcome this?

[dreamer-89]

This is interesting. Where do you see negative file length value @Rishikesh1159. @mch2 : Do you know when do we have negative file length value ?

[Rishikesh1159]

When running benchmarks I was logging size of files to fetch, I observed that with couple of replication events. I also observed time taken to process both "-1239252992" bytes size and "1239252992" bytes size is same. So, I was looking for it to convert to a positive value

[Rishikesh1159]

Sorry for confusion. Size of files to fetch will never be in negative. When I was testing locally I was using int type to store the size, so for bigger value int was overflowing and I saw negative size of files to fetch in logs. Later I used Long type and this is no more an issue.

[dreamer-89]

👍

[dreamer-89]

LGTM! Thanks @Rishikesh1159 for the quick fix. This is a critical change.

[dreamer-89]

nit: We can add a unit test to verify the timeouts here. I am fine taking up this in separate PR.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/2792/
• CommitID: c2db07d

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-4426-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 bff9d0fa6c5d56943a02344768ae37db54b1c71d
# Push it to GitHub
git push --set-upstream origin backport/backport-4426-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-4426-to-2.x.

[dreamer-89]

@Rishikesh1159 : Backport PR failed. Do you mind raising manual backport PR ?