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] Change remote_store setting validation message to make it more clear #4199

Merged
merged 3 commits into from
Aug 12, 2022

Conversation

sachinpkale
Copy link
Member

@sachinpkale sachinpkale commented Aug 12, 2022

Signed-off-by: Sachin Kale [email protected]

Description

  • Currently, if we try to enable index.remote_store.enabled without setting replication type to SEGMENT, we get following error:
{
  "error" : {
    "root_cause" : [
      {
        "type" : "illegal_argument_exception",
        "reason" : "Settings index.remote_store.enabled cannot be enabled when index.replication.type is set to DOCUMENT"
      }
    ],
    "type" : "illegal_argument_exception",
    "reason" : "Settings index.remote_store.enabled cannot be enabled when index.replication.type is set to DOCUMENT"
  },
  "status" : 400
}
  • But this does not make the dependency of segment replication clear. With this change, we are changing the error message to:
{
  "error" : {
    "root_cause" : [
      {
        "type" : "illegal_argument_exception",
        "reason" : "To enable index.remote_store.enabled, index.replication.type should be set to SEGMENT"
      }
    ],
    "type" : "illegal_argument_exception",
    "reason" : "To enable index.remote_store.enabled, index.replication.type should be set to SEGMENT"
  },
  "status" : 400
}

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 August 12, 2022 07:00
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@BhumikaSaini-Amazon
Copy link
Contributor

Thanks for adding this!
Please could you help update the unit tests as well for this change?

@sachinpkale
Copy link
Member Author

Please could you help update the unit tests as well for this change?

Forgot to update UTs. Thanks @BhumikaSaini-Amazon

Signed-off-by: Sachin Kale <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

Signed-off-by: Sachin Kale <[email protected]>
@BhumikaSaini-Amazon
Copy link
Contributor

Looks good to me 👍
Please link the respective Issue to this PR and strike off the PR checklist items
Thanks!

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@sachinpkale
Copy link
Member Author

Looks good to me 👍 Please link the respective Issue to this PR and strike off the PR checklist items Thanks!

@BhumikaSaini-Amazon Added issue link and updated PR checklist.

@Bukhtawar Bukhtawar merged commit 8ccedd8 into opensearch-project:main Aug 12, 2022
dreamer-89 pushed a commit to dreamer-89/OpenSearch that referenced this pull request Aug 12, 2022
… it more clear (opensearch-project#4199)

* Change remote_store setting validation message to make it more clear

Signed-off-by: Sachin Kale <[email protected]>
sachinpkale added a commit to sachinpkale/OpenSearch that referenced this pull request Sep 1, 2022
… it more clear (opensearch-project#4199)

* Change remote_store setting validation message to make it more clear

Signed-off-by: Sachin Kale <[email protected]>
sachinpkale added a commit to sachinpkale/OpenSearch that referenced this pull request Sep 2, 2022
… it more clear (opensearch-project#4199)

* Change remote_store setting validation message to make it more clear

Signed-off-by: Sachin Kale <[email protected]>
sachinpkale added a commit to sachinpkale/OpenSearch that referenced this pull request Sep 2, 2022
… it more clear (opensearch-project#4199)

* Change remote_store setting validation message to make it more clear

Signed-off-by: Sachin Kale <[email protected]>
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 <[email protected]>

Co-authored-by: Sachin Kale <[email protected]>

* Add index level setting for remote store

Signed-off-by: Sachin Kale <[email protected]>

Co-authored-by: Sachin Kale <[email protected]>

* Add RemoteDirectoryFactory and use RemoteDirectory instance in RefreshListener

Co-authored-by: Sachin Kale <[email protected]>
Signed-off-by: Sachin Kale <[email protected]>

* Upload segment to remote store post refresh

Signed-off-by: Sachin Kale <[email protected]>

Co-authored-by: Sachin Kale <[email protected]>
Signed-off-by: Sachin Kale <[email protected]>

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

* Inject remote store in IndexShard instead of RemoteStoreRefreshListener

Signed-off-by: Sachin Kale <[email protected]>

* Pass supplier of RepositoriesService to RemoteDirectoryFactory

Signed-off-by: Sachin Kale <[email protected]>

* Create isRemoteStoreEnabled function for IndexShard

Signed-off-by: Sachin Kale <[email protected]>

* Explicitly close remoteStore on indexShard close

Signed-off-by: Sachin Kale <[email protected]>

* Change RemoteDirectory.close to a no-op

Signed-off-by: Sachin Kale <[email protected]>

Co-authored-by: Sachin Kale <[email protected]>

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

* Add remote restore API implementation

Signed-off-by: Sachin Kale <[email protected]>

* [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 <[email protected]>

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

* Add rest endpoint for remote store restore

Signed-off-by: Sachin Kale <[email protected]>

* [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 <[email protected]>

* [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 <[email protected]>

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

* Add RemoteSegmentStoreDirectory to interact with remote segment store

Signed-off-by: Sachin Kale <[email protected]>

* Use RemoteSegmentStoreDirectory instead of RemoteDirectory (#4240)

* Use RemoteSegmentStoreDirectory instead of RemoteDirectory

Signed-off-by: Sachin Kale <[email protected]>

Signed-off-by: Sachin Kale <[email protected]>
Co-authored-by: Sachin Kale <[email protected]>
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.

3 participants