Remove deprecated S3 settings #24445
Conversation
These settings are deprecated in 5.5. This change removes them for 6.0.
There was a problem hiding this comment.
I left some minor comments. LGTM and nice to see all that code lighter than it was!
If you plan to add something in the breaking changes, you can may be take some of the stuff I initially wrote at https://github.com/elastic/elasticsearch/pull/23276/files#diff-c710699b84701013d83a5fe7ddc9a6d3.
| @@ -81,11 +81,11 @@ | |||
|
|
|||
| /** The number of retries to use when an s3 request fails. */ | |||
| static final Setting.AffixSetting<Integer> MAX_RETRIES_SETTING = Setting.affixKeySetting(PREFIX, "max_retries", | |||
| key -> Setting.intSetting(key, S3Repository.Repositories.MAX_RETRIES_SETTING, 0, Property.NodeScope)); | |||
| key -> Setting.intSetting(key, 3, 0, Property.NodeScope)); | |||
There was a problem hiding this comment.
I wonder if we should default to ClientConfiguration.DEFAULT_RETRY_POLICY.getMaxErrorRetry() instead?
Or to PredefinedRetryPolicies.DEFAULT_MAX_ERROR_RETRY?
| @@ -58,149 +58,67 @@ | |||
| * NOTE: These are legacy settings. Use the named client config settings above. | |||
| */ | |||
| public interface Repositories { | |||
There was a problem hiding this comment.
Not part of this change but you can remove public here. Also for public static final String TYPE = "s3";
| basePath = basePath.substring(1); | ||
| deprecationLogger.deprecated("S3 repository base_path trimming the leading `/`, and " + | ||
| "leading `/` will not be supported for the S3 repository in future releases"); | ||
| } | ||
| this.basePath = new BlobPath().add(basePath); | ||
| } else { | ||
| this.basePath = BlobPath.cleanPath(); |
There was a problem hiding this comment.
Not related to this change but method public static <T> T getValue(...) at the end of this class can become package private.
|
Thanks for the review @dadoonet. I pushed new commits with cleanups as you suggested, and also migration docs. Let me know if you are happy with the docs. |
|
Great! Thanks! |
|
@elasticmachine retest this please |
clintongormley
changed the title
clintongormley
added
:Distributed Coordination/Snapshot/Restore
We removed the global `repositories.s3.base_path` setting in 6.0 but it is still mentioned in the docs for the S3 repository plugin. This commit removes it from the docs. Relates elastic#24445
We removed the global `repositories.s3.base_path` setting in 6.0 but it is still mentioned in the docs for the S3 repository plugin. This commit removes it from the docs. Relates #24445
We removed the global `repositories.s3.base_path` setting in 6.0 but it is still mentioned in the docs for the S3 repository plugin. This commit removes it from the docs. Relates #24445
We removed the global `repositories.s3.base_path` setting in 6.0 but it is still mentioned in the docs for the S3 repository plugin. This commit removes it from the docs. Relates #24445
We removed the global `repositories.s3.base_path` setting in 6.0 but it is still mentioned in the docs for the S3 repository plugin. This commit removes it from the docs. Relates #24445
We removed the global `repositories.s3.base_path` setting in 6.0 but it is still mentioned in the docs for the S3 repository plugin. This commit removes it from the docs. Relates #24445
These settings are deprecated in 5.5. This change removes them for 6.0.