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

[Remote Store] Add RemoteSegmentStoreDirectory to interact with remote segment store #4020

Merged

Conversation

sachinpkale
Copy link
Member

@sachinpkale sachinpkale commented Jul 27, 2022

Signed-off-by: Sachin Kale kalsac@amazon.com

Description

  • To avoid concurrency issues where two primary exists for a given shard at the same time and upload segment files which can overwrite each other, we decided to add a unique suffix (UUID) to each segment filename that is uploaded to remote segment store.
  • But when we restore these segment files, we need restore them as per their original name. Also, need a mechanism to understand which segment file is a part of particular commit checkpoint. For this, we also upload a metadata file per checkpoint (refresh/commit). This metadata file contains map of original segment name to uploaded segment name.
  • The above logic is implemented in RemoteSegmentStoreDirectory which composes two instances of remote directories (one for segment and another for metadata) and still provides a directory interface for a caller. This way, caller would invoke directory methods of RemoteSegmentStoreDirectory in the same way as FSDirectory.
  • Two instances of RemoteDirectory that are part of RemoteSegmentStoreDirectory:
    • remoteDataDirectory: <Cluster UUID>/<Index UUID>/<Shard ID>/segments/data
    • remoteMetadataDirectory: <Cluster UUID>/<Index UUID>/<Shard ID>/segments/metadata
  • Sample Files under each directory path:
    • remoteMetadataDirectory
      • refresh_mapping__1__z__lKDiNIIBrs0AUNsRcOa3
      • commit_mapping__1__z__lKDiNIIBrs0AUNsRcOa3
      • refresh_mapping__1__y__g6DeNIIBrs0AUNsRQubk
      • commit_mapping__1__y__g6DeNIIBrs0AUNsRQubk
    • remoteDataDirectory
      • _10v.cfe__yXvPNIIBrs0AUNsRUXfQ
      • _10v.cfs__uHvPNIIBrs0AUNsRUVA2
      • _10v.si__h3vPNIIBrs0AUNsRSgJ-
      • _10w.cfe__UnrPNIIBrs0AUNsRR43c
      • _10w.cfs__ZXrPNIIBrs0AUNsRSbQU
      • _10w.si__KnrPNIIBrs0AUNsRRD-K

Issues Resolved

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.

@sachinpkale sachinpkale requested review from a team and reta as code owners July 27, 2022 04:17
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sachinpkale sachinpkale force-pushed the remote_segment_store_directory branch from 2602517 to 505acf9 Compare July 27, 2022 04:33
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sachinpkale sachinpkale force-pushed the remote_segment_store_directory branch from 505acf9 to fc25eaf Compare July 27, 2022 06:51
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sachinpkale sachinpkale force-pushed the remote_segment_store_directory branch from fc25eaf to 6e053b4 Compare July 28, 2022 08:57
@sachinpkale
Copy link
Member Author

@andrross @Bukhtawar Please review.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2022

Codecov Report

Merging #4020 (2470f4a) into main (a469a3c) will increase coverage by 0.04%.
The diff coverage is 86.59%.

@@             Coverage Diff              @@
##               main    #4020      +/-   ##
============================================
+ Coverage     70.59%   70.64%   +0.04%     
- Complexity    57083    57118      +35     
============================================
  Files          4603     4605       +2     
  Lines        274551   274670     +119     
  Branches      40210    40223      +13     
============================================
+ Hits         193831   194037     +206     
+ Misses        64514    64378     -136     
- Partials      16206    16255      +49     
Impacted Files Coverage Δ
...ava/org/opensearch/client/RestHighLevelClient.java 44.32% <ø> (-0.16%) ⬇️
...gregations/metrics/GeoBoundsAggregatorFactory.java 88.88% <ø> (ø)
...search/aggregations/metrics/InternalGeoBounds.java 66.66% <ø> (ø)
...o/search/aggregations/metrics/ParsedGeoBounds.java 88.00% <ø> (ø)
.../main/java/org/opensearch/search/SearchModule.java 96.27% <ø> (-0.03%) ⬇️
...earch/search/aggregations/AggregationBuilders.java 46.15% <ø> (+1.15%) ⬆️
...regations/support/AggregationInspectionHelper.java 53.65% <ø> (+1.27%) ⬆️
...g/opensearch/test/InternalAggregationTestCase.java 98.21% <ø> (-0.46%) ⬇️
...a/org/opensearch/test/OpenSearchIntegTestCase.java 57.37% <ø> (-0.11%) ⬇️
...regations/metrics/AbstractGeoBoundsAggregator.java 56.52% <56.52%> (ø)
... and 477 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Bukhtawar
Copy link
Collaborator

/cc : @ashking94 for help with review

private final RemoteDirectory remoteMetadataDirectory;

/**
* To prevent explosion of refresh metadata files, we replace refresh files for the given primary term and generation
Copy link
Member

Choose a reason for hiding this comment

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

Do you mean collision here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Idea is to keep count of metadata files in check as we will be calling listAll on metadataDirectory in order to get the list of segments.
Each commit will be followed by many refreshes. We don't want to add one metadata file per refresh. We will just replace the previous one. This works as we still have unique filenames for commit and scales as commits are not as frequent as refreshes.

* Returns names of files with given prefix in this directory.
* @param filenamePrefix The prefix to match against file names in the directory
* @return A list of the matching filenames in the directory
* @throws IOException IOException if there were any failures in reading from the blob container
Copy link
Member

Choose a reason for hiding this comment

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

one IOException can be removed from java doc?

private final String checksum;

UploadedSegmentMetadata(String originalFilename, String uploadedFilename, String checksum) {
this.originalFilename = originalFilename;
Copy link
Member

Choose a reason for hiding this comment

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

Should we add assertions here to be sure that these are valid values?

Copy link
Member Author

Choose a reason for hiding this comment

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

UploadedSegmentMetadata is a POJO which contains 3 strings. IMO, It doesn't/shouldn't do any validation.


public static UploadedSegmentMetadata fromString(String uploadedFilename) {
String[] values = uploadedFilename.split(SEPARATOR);
return new UploadedSegmentMetadata(values[0], values[1], values[2]);
Copy link
Member

@ashking94 ashking94 Jul 29, 2022

Choose a reason for hiding this comment

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

Should we handle exception use cases? Ideally a user should not be putting any other file than what the Opensearch remote store is putting in the concerned prefix. However, is there any behaviour if we see dubious files?

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above comment. UploadedSegmentMetadata should not validate this. The caller, on the other hand, should.

* another instance of {@code RemoteDirectory}.
* @opensearch.internal
*/
public final class RemoteSegmentStoreDirectory extends FilterDirectory {
Copy link
Member

Choose a reason for hiding this comment

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

Given there are 2 directories where the calls can be delegated, what value addition does FilterDirectory provides us here? Could we have extended Directory directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Extending directory would require most of the abstract methods to be re-implemented where we will be delegating it mostly to the data directory.

If you check RemoteDirectory class, most of the methods are throwing unsupported exception. Once we have the implementation for those methods and we think some pre-processing is required before delegating, we need to implement them in this class as well.

Copy link
Member

Choose a reason for hiding this comment

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

Quoting from FilterDirectory javadoc:

if you plan to write your own Directory implementation, you should consider extending directly Directory or BaseDirectory rather than try to reuse functionality of existing Directorys by extending this class.

Based on this, we should extend FilterDirectory only if we plan to use the Directory implementation. But in this case, we're not really using Directory implementation. Again, any benefit we're getting here by extending FilterDirectory?

Copy link
Member Author

Choose a reason for hiding this comment

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

By extending FilterDirectory, we don't have to implement all the methods that are present in the Directory. We delegate most of the methods directly to remoteDataDirectory. For the methods that need pre/post processing, they are implemented in this class. The first line of javadoc of FilterDirectory says:

Directory implementation that delegates calls to another directory.

This is exactly what we want.

if (latestRefreshMetadataFile.isPresent()) {
logger.info("Reading latest refresh metadata file {}", latestRefreshMetadataFile.get());
segmentMetadataMap.putAll(readMetadataFile(latestRefreshMetadataFile.get()));
this.refreshMetadataFileUniqueSuffix = MetadataFilenameUtils.getUuid(latestRefreshMetadataFile.get());
Copy link
Member

Choose a reason for hiding this comment

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

This method from the looks is only supposed to return the value that is eventually set to a class variable. Is it possible to set refreshMetadataFileUniqueSuffix outside this block and one level above as the segmentsUploadedToRemoteStore variable.

Copy link
Member

Choose a reason for hiding this comment

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

Also, the name of the method is readXXX. May be we can rename to something like readXXXAndUpdateYYY.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, this method should not be assigning refreshMetadataFileUniqueSuffix, will get this changed. But the goal of this method is to read only.

Copy link
Member

Choose a reason for hiding this comment

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

In what use case would we need to update refreshMetadataFileUniqueSuffix as part for this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was changed as part of latest commit.

}

/**
* Returns list of all the segment files uploaded to remote segment store till the last refresh checkpoint.
Copy link
Member

Choose a reason for hiding this comment

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

the method only returns the latest segment files from the last known commit (remotely) and the incremental refreshed segment files. Seems a bit misleading from the description that Returns list of all the segment files uploaded to remote segment store till the last refresh checkpoint.which sounds like all segment files ever.

Do we foresee where all we intend to use this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

commit metadata contains all the segment files till that point.

This method will be used in restore flow where we get the list of segment files from remote store and use it to copy to local directory.

public long fileLength(String name) throws IOException {
String remoteFilename = getExistingRemoteFilename(name);
if (remoteFilename != null) {
return remoteDataDirectory.fileLength(remoteFilename);
Copy link
Member

Choose a reason for hiding this comment

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

remoteDataDirectory.fileLength throws NoSuchFileException(name). You might want to handle the exception here and not return the internal filename outside?

Copy link
Member

Choose a reason for hiding this comment

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

there are more occurrences where this would be applicable. Pls look into those as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

But we are not exposing the internal filename, right? name is passed to this method.

* @throws IOException in case of I/O error
*/
@Override
public IndexOutput createOutput(String name, IOContext context) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is not really throwing an IOException nor any other exception. We can remove throws part where we are not really throwing exceptions.

Copy link
Member Author

Choose a reason for hiding this comment

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

remoteDataDirectory.createOutput throws IOException.

if (segmentsUploadedToRemoteStore.containsKey(localFilename)) {
return segmentsUploadedToRemoteStore.get(localFilename).uploadedFilename;
} else {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Could we have a fallback to remote store here in the else part if the local cache for whatsoever reason does not contain the name in the map?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this flow on purpose. This could result in a lot of load on the remote store itself.

@sachinpkale
Copy link
Member Author

Update: I got to know that SegmentInfosSnapshot does not just contain incremental segment files since the last commit but contain list of all the live segment files for the given shard. This would simplify current approach where instead of keeping track of 2 separate metadata files (commit and refresh), we keep track of only one metadata file. This is just a thought as of now. I will make the changes after outlining all the details.

@sachinpkale sachinpkale changed the title Add RemoteSegmentStoreDirectory to interact with remote segment store [Remote Store] Add RemoteSegmentStoreDirectory to interact with remote segment store Aug 1, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

Gradle Check (Jenkins) Run Completed with:

@sachinpkale
Copy link
Member Author

Build failed with:

java.lang.AssertionError: Failure at [repository_s3/20_repository_permanent_credentials:152]: expected [2xx] status code but api [snapshot.create] returned [400 Bad Request] [{"error":{"root_cause":[{"type":"invalid_snapshot_name_exception","reason":"[repository_permanent:snapshot-one] Invalid snapshot name [snapshot-one], snapshot with the same name already exists","stack_trace":"InvalidSnapshotNameException[[repository_permanent:snapshot-one] Invalid snapshot name [snapshot-one], snapshot with the same name already exists]

This test is not related with the changes in this PR. Re-triggering the build.

@sachinpkale sachinpkale force-pushed the remote_segment_store_directory branch from 813dc00 to 522905e Compare August 1, 2022 06:06
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2022

Gradle Check (Jenkins) Run Completed with:

@dreamer-89
Copy link
Member

Created #4069 to track this.

@sachinpkale sachinpkale force-pushed the remote_segment_store_directory branch from 522905e to 2efcc10 Compare August 2, 2022 02:07
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

Gradle Check (Jenkins) Run Completed with:

@sachinpkale sachinpkale force-pushed the remote_segment_store_directory branch from 2efcc10 to 1a4cbda Compare August 2, 2022 05:12
* another instance of {@code RemoteDirectory}.
* @opensearch.internal
*/
public final class RemoteSegmentStoreDirectory extends FilterDirectory {
Copy link
Member

Choose a reason for hiding this comment

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

Quoting from FilterDirectory javadoc:

if you plan to write your own Directory implementation, you should consider extending directly Directory or BaseDirectory rather than try to reuse functionality of existing Directorys by extending this class.

Based on this, we should extend FilterDirectory only if we plan to use the Directory implementation. But in this case, we're not really using Directory implementation. Again, any benefit we're getting here by extending FilterDirectory?

* Each segment file is uploaded with unique suffix.
* For example, _0.cfe in local filesystem will be uploaded to remote segment store as _0.cfe__gX7bNIIBrs0AUNsR2yEG
*/
public static final String SEGMENT_NAME_UUID_SEPARATOR = "__";
Copy link
Member

Choose a reason for hiding this comment

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

Can the segment name or UUID also contain __ ? Just wondering if we can run into corner cases where the segment name translation can break.

Copy link
Member Author

Choose a reason for hiding this comment

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

UUID should not contain special character other than -. As per lucene file format explained here: https://lucene.apache.org/core/3_0_3/fileformats.html#File%20Naming, __ should not be included in the segment name. But I agree with the concern. Not sure which delimiter can be used which will guarantee that it will not be used in the segment name.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Should be fine then. First occurrence is always guaranteed to be the separator.

*/
public void init() throws IOException {
this.refreshMetadataFileUniqueSuffix = UUIDs.base64UUID();
this.segmentsUploadedToRemoteStore = new ConcurrentHashMap<>(readLatestMetadataFile());
Copy link
Member

Choose a reason for hiding this comment

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

We haven't really uploaded any segments to remote store at this point right? or does this handle the cases where replica gets promoted to primary?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, failover or node restart.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sachinpkale it seems like there is no cleanup mechanism in place: if init() is called for initialized bu stale cache, the stale files have to be cleaned up (using previous metadataFileUniqueSuffix).

Copy link
Member Author

Choose a reason for hiding this comment

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

@reta We always read latest metadata file. So, even if there are more than 1 metadata files, it won't impact the correctness.
But you are right on cleanup part. We need cleanup of stale metadata files in order to keep performance of init() in check. I will be raising a follow-up PR which will contain clean-up logic. Implementation is part of another branch: sachinpkale@b0d95c9#diff-f57628264c235a133d2d6db458404e07a43f18026e40ed5e956536c03add9925R273

if (latestRefreshMetadataFile.isPresent()) {
logger.info("Reading latest refresh metadata file {}", latestRefreshMetadataFile.get());
segmentMetadataMap.putAll(readMetadataFile(latestRefreshMetadataFile.get()));
this.refreshMetadataFileUniqueSuffix = MetadataFilenameUtils.getUuid(latestRefreshMetadataFile.get());
Copy link
Member

Choose a reason for hiding this comment

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

In what use case would we need to update refreshMetadataFileUniqueSuffix as part for this method?

@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

Gradle Check (Jenkins) Run Completed with:

@sachinpkale sachinpkale force-pushed the remote_segment_store_directory branch from 1a4cbda to 1ef8034 Compare August 2, 2022 11:20
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

Gradle Check (Jenkins) Run Completed with:

@sachinpkale sachinpkale force-pushed the remote_segment_store_directory branch from 1ef8034 to f9c9430 Compare August 2, 2022 13:29
@github-actions
Copy link
Contributor

github-actions bot commented Aug 2, 2022

Gradle Check (Jenkins) Run Completed with:

Comment on lines +55 to +76
private final RemoteDirectory remoteDataDirectory;
/**
* remoteMetadataDirectory is used to store metadata files at path: cluster_UUID/index_UUID/shardId/segments/metadata
*/
private final RemoteDirectory remoteMetadataDirectory;

/**
* To prevent explosion of refresh metadata files, we replace refresh files for the given primary term and generation
* This is achieved by uploading refresh metadata file with the same UUID suffix.
*/
private String metadataFileUniqueSuffix;

/**
* Keeps track of local segment filename to uploaded filename along with other attributes like checksum.
* This map acts as a cache layer for uploaded segment filenames which helps avoid calling listAll() each time.
* It is important to initialize this map on creation of RemoteSegmentStoreDirectory and update it on each upload and delete.
*/
private Map<String, UploadedSegmentMetadata> segmentsUploadedToRemoteStore;

private static final Logger logger = LogManager.getLogger(RemoteSegmentStoreDirectory.class);

public RemoteSegmentStoreDirectory(RemoteDirectory remoteDataDirectory, RemoteDirectory remoteMetadataDirectory) throws IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The only concern I see with having two RemoteDirectory instances is they can fundamentally be two separate stores altogether with varying consistency properties. Do we really need them to be exposed separately. Can we not keep the metadata repository internal to this class, to avoid possible misuse of this class.

Copy link
Member Author

Choose a reason for hiding this comment

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

remoteMetadataDirectory will be internal to this class. Callers of RemoteSegmentStoreDirectory will not be aware of the existence of two different remote directories.

On that part of creating an instance of RemoteSegmentStoreDirectory, it will be handled by the factory. The implementation of factory method will be part of subsequent PR but you can find the code in this commit: https://github.com/sachinpkale/OpenSearch/blob/remote_segment_failure_handling/server/src/main/java/org/opensearch/index/store/RemoteSegmentStoreDirectoryFactory.java

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure but the uploadMetadata is still public

 public void uploadMetadata(Collection<String> segmentFiles, Directory storeDirectory, long primaryTerm, long generation)

Copy link
Member Author

@sachinpkale sachinpkale Aug 8, 2022

Choose a reason for hiding this comment

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

Yes, I think you are suggesting to make uploadMetadata private. That is that plan but I haven't made that change as not sure on how to decide the trigger for metadata upload.
Today, it will be called by RemoteStoreRefereshListener as RemoteStoreRefreshListener knows when the refresh is happening.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Bukhtawar Created a tracking issue for the same: #4171

Sachin Kale added 2 commits August 12, 2022 11:12
Signed-off-by: Sachin Kale <kalsac@amazon.com>
Signed-off-by: Sachin Kale <kalsac@amazon.com>
@sachinpkale sachinpkale force-pushed the remote_segment_store_directory branch from ce356bf to dafb962 Compare August 12, 2022 05:43
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sachinpkale
Copy link
Member Author

Failing tests:

REPRODUCE WITH: ./gradlew ':server:internalClusterTest' --tests "org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testRestoreSnapshotAllocationDoesNotExceedWatermark" -Dtests.seed=DFD6B4D068693FA8 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=en-PH -Dtests.timezone=ROK -Druntime.java=17

Seems like a flaky test. Re-triggering build

Signed-off-by: Sachin Kale <kalsac@amazon.com>
@sachinpkale sachinpkale force-pushed the remote_segment_store_directory branch from dafb962 to 2470f4a Compare August 12, 2022 06:46
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Comment on lines +239 to +246
@Override
public void deleteFile(String name) throws IOException {
String remoteFilename = getExistingRemoteFilename(name);
if (remoteFilename != null) {
remoteDataDirectory.deleteFile(remoteFilename);
segmentsUploadedToRemoteStore.remove(name);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

One rare race could be we delete file and segmentsUploadedToRemoteStore hasn't been updated and then a concurrent upload happens and we check for containsFile where the stale entry that the file is uploaded is present, will cause the new segment file to get skipped from being uploaded.
Can you confirm if there is a valid scenario that might hit this race?

Copy link
Member Author

@sachinpkale sachinpkale Aug 12, 2022

Choose a reason for hiding this comment

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

I don't think of a scenario where this should happen due to following reasons:

  1. Segment file with the same name is not created, even after segments are deleted as part of merge, new segment files will always be prefixed with next generation number.
  2. Even if segment file with the same name is re-created (this contradicts with point number 1), we make sure each segment file is uploaded with the unique suffix.

@Bukhtawar Bukhtawar merged commit 5f2e66b into opensearch-project:main Aug 13, 2022
sachinpkale added a commit to sachinpkale/OpenSearch that referenced this pull request Sep 1, 2022
…e segment store (opensearch-project#4020)

* Add RemoteSegmentStoreDirectory to interact with remote segment store

Signed-off-by: Sachin Kale <kalsac@amazon.com>
sachinpkale added a commit to sachinpkale/OpenSearch that referenced this pull request Sep 2, 2022
…e segment store (opensearch-project#4020)

* Add RemoteSegmentStoreDirectory to interact with remote segment store

Signed-off-by: Sachin Kale <kalsac@amazon.com>
sachinpkale added a commit to sachinpkale/OpenSearch that referenced this pull request Sep 2, 2022
…e segment store (opensearch-project#4020)

* Add RemoteSegmentStoreDirectory to interact with remote segment store

Signed-off-by: Sachin Kale <kalsac@amazon.com>
Bukhtawar pushed a commit that referenced this pull request Sep 2, 2022
…4380)

* [Remote Store] Upload segments to remote store post refresh (#3460)

* Add RemoteDirectory interface to copy segment files to/from remote store

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Co-authored-by: Sachin Kale <kalsac@amazon.com>

* Add index level setting for remote store

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Co-authored-by: Sachin Kale <kalsac@amazon.com>

* Add RemoteDirectoryFactory and use RemoteDirectory instance in RefreshListener

Co-authored-by: Sachin Kale <kalsac@amazon.com>
Signed-off-by: Sachin Kale <kalsac@amazon.com>

* Upload segment to remote store post refresh

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Co-authored-by: Sachin Kale <kalsac@amazon.com>
Signed-off-by: Sachin Kale <kalsac@amazon.com>

* [Remote Store] Inject remote store in IndexShard instead of RemoteStoreRefreshListener (#3703)

* Inject remote store in IndexShard instead of RemoteStoreRefreshListener

Signed-off-by: Sachin Kale <kalsac@amazon.com>

* Pass supplier of RepositoriesService to RemoteDirectoryFactory

Signed-off-by: Sachin Kale <kalsac@amazon.com>

* Create isRemoteStoreEnabled function for IndexShard

Signed-off-by: Sachin Kale <kalsac@amazon.com>

* Explicitly close remoteStore on indexShard close

Signed-off-by: Sachin Kale <kalsac@amazon.com>

* Change RemoteDirectory.close to a no-op

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Co-authored-by: Sachin Kale <kalsac@amazon.com>

* [Remote Store] Add remote store restore API implementation (#3642)

* Add remote restore API implementation

Signed-off-by: Sachin Kale <kalsac@amazon.com>

* [Remote Store] Add support to add nested settings for remote store (#4060)

* Add support to add nested settings for remote store

Signed-off-by: Sachin Kale <kalsac@amazon.com>

* [Remote Store] Add rest endpoint for remote store restore (#3576)

* Add rest endpoint for remote store restore

Signed-off-by: Sachin Kale <kalsac@amazon.com>

* [Remote Store] Add validator that forces segment replication type before enabling remote store (#4175)

* Add validator that forces segment replication type before enabling remote store

Signed-off-by: Sachin Kale <kalsac@amazon.com>

* [Remote Store] Change remote_store setting validation message to make it more clear (#4199)

* Change remote_store setting validation message to make it more clear

Signed-off-by: Sachin Kale <kalsac@amazon.com>

* [Remote Store] Add RemoteSegmentStoreDirectory to interact with remote segment store (#4020)

* Add RemoteSegmentStoreDirectory to interact with remote segment store

Signed-off-by: Sachin Kale <kalsac@amazon.com>

* Use RemoteSegmentStoreDirectory instead of RemoteDirectory (#4240)

* Use RemoteSegmentStoreDirectory instead of RemoteDirectory

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Signed-off-by: Sachin Kale <kalsac@amazon.com>
Co-authored-by: Sachin Kale <kalsac@amazon.com>
andrross added a commit to andrross/OpenSearch that referenced this pull request Jun 12, 2023
I have nominated and maintainers have agreed to invite Sachin Kale
(@sachinpkale) to be a co-maintainer. Sachin has kindly accepted.

Sachin has led the design and implementation of the remote backed
storage feature in OpenSearch. This feature was introduced as
experimental in OpenSearch 2.3 and is planned for general availability
in 2.9. Some significant issues and PRs authored by Sachin for this
effort are as follows:

Feature proposal: opensearch-project#1968
Upload segments to remote store post refresh: opensearch-project#3460
Add rest endpoint for remote store restore: opensearch-project#3576
Add RemoteSegmentStoreDirectory to interact with remote segment store: opensearch-project#4020

In total, Sachin has authored 57 PRs going back to May 2022. He also
frequently reviews contributions from others and has reviewed nearly 100
PRs in the same time frame.

Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross added a commit to andrross/OpenSearch that referenced this pull request Jun 12, 2023
I have nominated and maintainers have agreed to invite Sachin Kale
(@sachinpkale) to be a co-maintainer. Sachin has kindly accepted.

Sachin has led the design and implementation of the remote backed
storage feature in OpenSearch. This feature was introduced as
experimental in OpenSearch 2.3 and is planned for general availability
in 2.9. Some significant issues and PRs authored by Sachin for this
effort are as follows:

Feature proposal: opensearch-project#1968
Upload segments to remote store post refresh: opensearch-project#3460
Add rest endpoint for remote store restore: opensearch-project#3576
Add RemoteSegmentStoreDirectory to interact with remote segment store: opensearch-project#4020

In total, Sachin has authored 57 PRs going back to May 2022. He also
frequently reviews contributions from others and has reviewed nearly 100
PRs in the same time frame.

Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross added a commit to andrross/OpenSearch that referenced this pull request Jun 12, 2023
I have nominated and maintainers have agreed to invite Sachin Kale
(@sachinpkale) to be a co-maintainer. Sachin has kindly accepted.

Sachin has led the design and implementation of the remote backed
storage feature in OpenSearch. This feature was introduced as
experimental in OpenSearch 2.3 and is planned for general availability
in 2.9. Some significant issues and PRs authored by Sachin for this
effort are as follows:

Feature proposal: opensearch-project#1968
Upload segments to remote store post refresh: opensearch-project#3460
Add rest endpoint for remote store restore: opensearch-project#3576
Add RemoteSegmentStoreDirectory to interact with remote segment store: opensearch-project#4020

In total, Sachin has authored 57 PRs going back to May 2022. He also
frequently reviews contributions from others and has reviewed nearly 100
PRs in the same time frame.

Signed-off-by: Andrew Ross <andrross@amazon.com>
kotwanikunal pushed a commit that referenced this pull request Jun 12, 2023
I have nominated and maintainers have agreed to invite Sachin Kale
(@sachinpkale) to be a co-maintainer. Sachin has kindly accepted.

Sachin has led the design and implementation of the remote backed
storage feature in OpenSearch. This feature was introduced as
experimental in OpenSearch 2.3 and is planned for general availability
in 2.9. Some significant issues and PRs authored by Sachin for this
effort are as follows:

Feature proposal: #1968
Upload segments to remote store post refresh: #3460
Add rest endpoint for remote store restore: #3576
Add RemoteSegmentStoreDirectory to interact with remote segment store: #4020

In total, Sachin has authored 57 PRs going back to May 2022. He also
frequently reviews contributions from others and has reviewed nearly 100
PRs in the same time frame.

Signed-off-by: Andrew Ross <andrross@amazon.com>
andrross added a commit to andrross/OpenSearch that referenced this pull request Jun 13, 2023
I have nominated and maintainers have agreed to invite Sachin Kale
(@sachinpkale) to be a co-maintainer. Sachin has kindly accepted.

Sachin has led the design and implementation of the remote backed
storage feature in OpenSearch. This feature was introduced as
experimental in OpenSearch 2.3 and is planned for general availability
in 2.9. Some significant issues and PRs authored by Sachin for this
effort are as follows:

Feature proposal: opensearch-project#1968
Upload segments to remote store post refresh: opensearch-project#3460
Add rest endpoint for remote store restore: opensearch-project#3576
Add RemoteSegmentStoreDirectory to interact with remote segment store: opensearch-project#4020

In total, Sachin has authored 57 PRs going back to May 2022. He also
frequently reviews contributions from others and has reviewed nearly 100
PRs in the same time frame.

Signed-off-by: Andrew Ross <andrross@amazon.com>
saratvemulapalli pushed a commit that referenced this pull request Jun 13, 2023
I have nominated and maintainers have agreed to invite Sachin Kale
(@sachinpkale) to be a co-maintainer. Sachin has kindly accepted.

Sachin has led the design and implementation of the remote backed
storage feature in OpenSearch. This feature was introduced as
experimental in OpenSearch 2.3 and is planned for general availability
in 2.9. Some significant issues and PRs authored by Sachin for this
effort are as follows:

Feature proposal: #1968
Upload segments to remote store post refresh: #3460
Add rest endpoint for remote store restore: #3576
Add RemoteSegmentStoreDirectory to interact with remote segment store: #4020

In total, Sachin has authored 57 PRs going back to May 2022. He also
frequently reviews contributions from others and has reviewed nearly 100
PRs in the same time frame.

Signed-off-by: Andrew Ross <andrross@amazon.com>
gaiksaya pushed a commit to gaiksaya/OpenSearch that referenced this pull request Jun 26, 2023
opensearch-project#8047)

I have nominated and maintainers have agreed to invite Sachin Kale
(@sachinpkale) to be a co-maintainer. Sachin has kindly accepted.

Sachin has led the design and implementation of the remote backed
storage feature in OpenSearch. This feature was introduced as
experimental in OpenSearch 2.3 and is planned for general availability
in 2.9. Some significant issues and PRs authored by Sachin for this
effort are as follows:

Feature proposal: opensearch-project#1968
Upload segments to remote store post refresh: opensearch-project#3460
Add rest endpoint for remote store restore: opensearch-project#3576
Add RemoteSegmentStoreDirectory to interact with remote segment store: opensearch-project#4020

In total, Sachin has authored 57 PRs going back to May 2022. He also
frequently reviews contributions from others and has reviewed nearly 100
PRs in the same time frame.

Signed-off-by: Andrew Ross <andrross@amazon.com>
imRishN pushed a commit to imRishN/OpenSearch that referenced this pull request Jun 27, 2023
I have nominated and maintainers have agreed to invite Sachin Kale
(@sachinpkale) to be a co-maintainer. Sachin has kindly accepted.

Sachin has led the design and implementation of the remote backed
storage feature in OpenSearch. This feature was introduced as
experimental in OpenSearch 2.3 and is planned for general availability
in 2.9. Some significant issues and PRs authored by Sachin for this
effort are as follows:

Feature proposal: opensearch-project#1968
Upload segments to remote store post refresh: opensearch-project#3460
Add rest endpoint for remote store restore: opensearch-project#3576
Add RemoteSegmentStoreDirectory to interact with remote segment store: opensearch-project#4020

In total, Sachin has authored 57 PRs going back to May 2022. He also
frequently reviews contributions from others and has reviewed nearly 100
PRs in the same time frame.

Signed-off-by: Andrew Ross <andrross@amazon.com>
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
I have nominated and maintainers have agreed to invite Sachin Kale
(@sachinpkale) to be a co-maintainer. Sachin has kindly accepted.

Sachin has led the design and implementation of the remote backed
storage feature in OpenSearch. This feature was introduced as
experimental in OpenSearch 2.3 and is planned for general availability
in 2.9. Some significant issues and PRs authored by Sachin for this
effort are as follows:

Feature proposal: opensearch-project#1968
Upload segments to remote store post refresh: opensearch-project#3460
Add rest endpoint for remote store restore: opensearch-project#3576
Add RemoteSegmentStoreDirectory to interact with remote segment store: opensearch-project#4020

In total, Sachin has authored 57 PRs going back to May 2022. He also
frequently reviews contributions from others and has reviewed nearly 100
PRs in the same time frame.

Signed-off-by: Andrew Ross <andrross@amazon.com>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
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.

7 participants