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

Use DirectoryFactory interface to create remote directory #6970

Merged

Conversation

sachinpkale
Copy link
Member

@sachinpkale sachinpkale commented Apr 4, 2023

Description

  • Currently, we have a separate RemoteDirectoryFactory interface to create an instance of RemoteDirectory.
  • This was created as it required a separate Repository instance as an input. But in [Remote Store] Inject remote store in IndexShard instead of RemoteStoreRefreshListener #3703, we changed the parameter type from Repository to String (repository name).
  • As repository name is part of index settings and as index settings is one of the parameters to DirectoryFactory interface, RemoteDirectoryFactory becomes redundant.

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

Signed-off-by: Sachin Kale <kalsac@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Sachin Kale <kalsac@amazon.com>
@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2023

Gradle Check (Jenkins) Run Completed with:

Copy link
Member

@andrross andrross left a comment

Choose a reason for hiding this comment

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

Thanks @sachinpkale! I actually noticed this a couple months ago so thanks for doing this.

Is it possible/is there a plan to evolve this code so that IndexService doesn't need to hold two directory factories and a single implementation can do the local+remote mirroring functionality offered by the two factories now?

@andrross andrross added the backport 2.x Backport to 2.x branch label Apr 4, 2023
@andrross andrross merged commit 56a2bed into opensearch-project:main Apr 4, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 4, 2023
* Use DirectoryFactory interface to create remote directory

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

* Fix tests

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

---------

Signed-off-by: Sachin Kale <kalsac@amazon.com>
Co-authored-by: Sachin Kale <kalsac@amazon.com>
(cherry picked from commit 56a2bed)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
mitrofmep pushed a commit to mitrofmep/OpenSearch that referenced this pull request Apr 5, 2023
…-project#6970)

* Use DirectoryFactory interface to create remote directory

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

* Fix tests

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

---------

Signed-off-by: Sachin Kale <kalsac@amazon.com>
Co-authored-by: Sachin Kale <kalsac@amazon.com>
Signed-off-by: Valentin Mitrofanov <mitrofmep@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants