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

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

Merged
merged 3 commits into from
Sep 7, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8138702
Fix timeout issue by calculating time needed to process getSegmentFiles.
Rishikesh1159 Sep 6, 2022
3fb0e25
Formatting sizeOfSegmentFiles for time calculation.
Rishikesh1159 Sep 6, 2022
c2db07d
Addressing comments and applying spotless check.
Rishikesh1159 Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Fix NoSuchFileExceptions with segment replication when computing primary metadata snapshots ([#4366](https://github.com/opensearch-project/OpenSearch/pull/4366))
- [Segment Replication] Update flaky testOnNewCheckpointFromNewPrimaryCancelOngoingReplication unit test ([#4414](https://github.com/opensearch-project/OpenSearch/pull/4414))
- Fixed the `_cat/shards/10_basic.yml` test cases fix.
- [Segment Replication] Fix timeout issue by calculating time needed to process getSegmentFiles ([#4426](https://github.com/opensearch-project/OpenSearch/pull/4426))

### Security
- CVE-2022-25857 org.yaml:snakeyaml DOS vulnerability ([#4341](https://github.com/opensearch-project/OpenSearch/pull/4341))
Expand All @@ -71,4 +72,4 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)


[Unreleased]: https://github.com/opensearch-project/OpenSearch/compare/2.2.0...HEAD
[2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.2.0...2.x
[2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.2.0...2.x
25 changes: 24 additions & 1 deletion server/src/main/java/org/opensearch/indices/replication/PrimaryShardReplicationSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@
import org.opensearch.action.ActionListener;
import org.opensearch.cluster.node.DiscoveryNode;
import org.opensearch.common.io.stream.Writeable;
import org.opensearch.common.unit.TimeValue;
import org.opensearch.index.store.Store;
import org.opensearch.index.store.StoreFileMetadata;
import org.opensearch.indices.recovery.RecoverySettings;
import org.opensearch.indices.recovery.RetryableTransportClient;
import org.opensearch.indices.replication.checkpoint.ReplicationCheckpoint;
import org.opensearch.transport.TransportRequestOptions;
import org.opensearch.transport.TransportService;

import java.util.List;
Expand Down Expand Up @@ -78,14 +80,35 @@ public void getSegmentFiles(
) {
final Writeable.Reader<GetSegmentFilesResponse> reader = GetSegmentFilesResponse::new;
final ActionListener<GetSegmentFilesResponse> responseListener = ActionListener.map(listener, r -> r);
// Few of the below assumptions and calculations are added for experimental release of segment replication feature in 2.3
// version.These will be changed in next release.
dreamer-89 marked this conversation as resolved.
Show resolved Hide resolved

// Storing the size of files to fetch in bytes.
long sizeOfSegmentFiles = 0;
for (int i = 0; i < filesToFetch.size(); i++) {
sizeOfSegmentFiles += filesToFetch.get(i).length();
}
Copy link
Member

Choose a reason for hiding this comment

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

These can be reduced with below

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

// Making sure files size is in correct format to perform time calculation.
if(sizeOfSegmentFiles<0){
sizeOfSegmentFiles *=-1;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why this change is needed ?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Rishikesh1159 Rishikesh1159 Sep 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

👍

// Maximum size of files to fetch (segment files), that can be processed in 1 minute for a m5.xlarge machine.
long baseSegmentFilesSize = 300000000;
Copy link
Member

@dreamer-89 dreamer-89 Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

  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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


long timeToGetSegmentFiles = 1;
Copy link
Member

@dreamer-89 dreamer-89 Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Should this be initialized with 0 ?

Copy link
Member

Choose a reason for hiding this comment

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

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

// Formula for calculating time needed to process a replication event's files to fetch process
timeToGetSegmentFiles += sizeOfSegmentFiles / baseSegmentFilesSize;
Copy link
Member

@dreamer-89 dreamer-89 Sep 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can this be reduced with below ?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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"

final GetSegmentFilesRequest request = new GetSegmentFilesRequest(
replicationId,
targetAllocationId,
targetNode,
filesToFetch,
checkpoint
);
transportClient.executeRetryableAction(GET_SEGMENT_FILES, request, responseListener, reader);
final TransportRequestOptions options = TransportRequestOptions.builder()
.withTimeout(TimeValue.timeValueMinutes(timeToGetSegmentFiles))
.build();
transportClient.executeRetryableAction(GET_SEGMENT_FILES, request, options, responseListener, reader);
Copy link
Member

Choose a reason for hiding this comment

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

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

}

@Override
Expand Down