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

Streamline S3 Repository- and Client-Settings #37393

Merged

Conversation

Projects
None yet
4 participants
@original-brownbear
Copy link
Member

commented Jan 13, 2019

  • Make clusterstate settings override static settings
  • Make repository settings override client settings
  • Cache clients according to settings not name to prevent conflicts between multiple repositories using the same named client with different custom settings causing conflicts
    • Introduce custom implementations for the AWS credentials here to be able to use them as part of a hash key
Streamline S3 Repository- and Client-Settings
* Make clusterstate settings override static settings
* Make repository settings override client settings
* Cache clients according to settings
   * Introduce custom implementations for the AWS credentials here to be able to use them as part of a hash key
@elasticmachine

This comment has been minimized.

Copy link

commented Jan 13, 2019

@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

Jenkins run Gradle build tests 1

ByteSizeValue bufferSize, String cannedACL, String storageClass) {
private final Tuple<RepositoryMetaData, Settings> settingsKey;

S3BlobStore(S3Service service, String bucket, boolean serverSideEncryption,

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jan 14, 2019

Author Member

This constructor could be simplified now, making all the setting extraction from the metadata happen here. I just left that out of this step because it requires some noisy changes in the tests.

@ywelsch ywelsch self-requested a review Jan 14, 2019

@@ -51,15 +52,18 @@

private final StorageClass storageClass;

S3BlobStore(S3Service service, String clientName, String bucket, boolean serverSideEncryption,
ByteSizeValue bufferSize, String cannedACL, String storageClass) {
private final Tuple<RepositoryMetaData, Settings> settingsKey;

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jan 18, 2019

Contributor

this is perhaps a bit too coarse-grained as settings key? Should it just be repository metadata that's relevant for establishing connections?

Also, why are the settings (i.e. 2nd part of tuple) part of this key? These are static / fixed at node startup, so there's no need to cache them?

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jan 18, 2019

Author Member

@ywelsch

Also, why are the settings (i.e. 2nd part of tuple) part of this key? These are static / fixed at node startup, so there's no need to cache them?

I think I was a little paranoid here with reloads from the secret store but on second thought, that's wrong it's all handled by refreshAndClearCache just fine :). I'll simplify the key.

this is perhaps a bit too coarse-grained as settings key? Should it just be repository metadata that's relevant for establishing connections?

Maybe in theory, but does it matter in practice? Creating a new repo client isn't absurdly expensive and updating the repo settings is a rare event? I agree we could create another key here but it seems like it's just a bunch of extra code complexity and then if we ever add a new relevant (for establishing the connection) setting we have to maintain this as well => maybe the rare case of needlessly creating a new client isn't a problem?

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jan 18, 2019

Contributor

Maybe in theory, but does it matter in practice?

I was wondering if with this approach we never share clients between repo definitions (as these repos will probably be different in some way). Not sure how many repo definitions we typically have in clusters though and whether it matters.

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jan 18, 2019

Author Member

Wait :) I can't take credit for it now, but I think I actually handled this just fine already :D
The actual client instances are cached by their S3ClientSettings which only contain stuff needed for instantiating the clients.
So org.elasticsearch.repositories.s3.S3Service#clientsCache should deduplicate clients just fine. All we get are some redundant settings instances in org.elasticsearch.repositories.s3.S3Service#derivedClientSettings where two different repo-metadata keys could point to equal settings instances.

@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2019

@ywelsch ditched the redundant use of the static settings from the cache key and client settings loading in 8f2a85b

@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jan 18, 2019

Jenkins run Gradle build tests 1

@ywelsch ywelsch self-requested a review Jan 21, 2019

@ywelsch

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2019

Can you investigate how to test this PR?

@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

@ywelsch yes on it today tomorrow :)

@original-brownbear original-brownbear removed the WIP label Jan 22, 2019

@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

@ywelsch alright, added an IT that shows that we're not overriding the named clients anymore when using settings in the repository definition (I realize that it's less than elegant to test it the way I did not by breaking the repo, but our current IT infrastructure makes it extremely hard to pass in a correct endpoint here because we compute the replacements in the .yml files long before Minio starts up).
Also, added a UT that tests the basic workflow of overriding the settings, let me know if this is what you had in mind :)

@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2019

Added some short docs here 46acd77

@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2019

I just learnt that we could write really nice tests here if the ones I added are too hack using the test fixtures plugin. Looking into #37783 now.

Show resolved Hide resolved ...sitory-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java Outdated
clientName,
metadata.name());
}

if (S3ClientSettings.checkDeprecatedCredentials(metadata.settings())) {

This comment has been minimized.

Copy link
@ywelsch

ywelsch Jan 29, 2019

Contributor

should we also add deprecation warnings for all other client settings in REPO settings?

This comment has been minimized.

Copy link
@original-brownbear

original-brownbear Jan 29, 2019

Author Member

IDK, if I think about it it seems a little redundant:

  • We brought this back to life now, so it'll probably mostly be used by people getting the idea from the docs that say it's deprecated
  • As far as I understand this may not stay deprecated?

... but that said, I can add some if you want me to, I don't really have an opinion here :)

Show resolved Hide resolved ...sitory-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java Outdated

@original-brownbear original-brownbear merged commit 57823c4 into elastic:master Jan 30, 2019

8 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/docs-check Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@original-brownbear original-brownbear deleted the original-brownbear:repo-setting-fixes branch Jan 30, 2019

@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

@ywelsch thanks for the review! Merged this now so I can get going on the back port, but let me know if I should add a PR for the deprecation logging :)

original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Jan 30, 2019

Streamline S3 Repository- and Client-Settings (elastic#37393)
* Make repository settings override static settings
* Cache clients according to settings
   * Introduce custom implementations for the AWS credentials here to be able to use them as part of a hash key

original-brownbear added a commit that referenced this pull request Jan 30, 2019

Streamline S3 Repository- and Client-Settings (#37393) (#38010)
* Make repository settings override static settings
* Cache clients according to settings
   * Introduce custom implementations for the AWS credentials here to be able to use them as part of a hash key
@original-brownbear

This comment has been minimized.

Copy link
Member Author

commented Jan 30, 2019

Backported to 6.x in #38010

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 30, 2019

Merge branch 'master' into retention-lease-stats
* master: (29 commits)
  Fix limit on retaining sequence number (elastic#37992)
  Docs test fix, wait for shards active.
  Revert "Revert "Documented default values for index follow request parameters. (elastic#37917)""
  Revert "Documented default values for index follow request parameters. (elastic#37917)"
  Ensure date parsing BWC compatibility (elastic#37929)
  SQL: Skip the nested and object field types in case of an ODBC request (elastic#37948)
  Use mappings to format doc-value fields by default. (elastic#30831)
  Give precedence to index creation when mixing typed templates with typeless index creation and vice-versa. (elastic#37871)
  Add classifier to tar.gz in docker compose (elastic#38011)
  Documented default values for index follow request parameters. (elastic#37917)
  Fix fetch source option in expand search phase (elastic#37908)
  Restore a noop _all metadata field for 6x indices (elastic#37808)
  Added ccr to xpack usage infrastructure (elastic#37256)
  Fix exit code for Security CLI tools  (elastic#37956)
  Streamline S3 Repository- and Client-Settings (elastic#37393)
  Add version 6.6.1 (elastic#37975)
  Ensure task metadata not null in follow test (elastic#37993)
  Docs fix - missing callout
  Types removal - deprecate include_type_name with index templates (elastic#37484)
  Handle completion suggestion without contexts
  ...

henningandersen added a commit to henningandersen/elasticsearch that referenced this pull request Jan 30, 2019

Streamline S3 Repository- and Client-Settings (elastic#37393)
* Make repository settings override static settings
* Cache clients according to settings
   * Introduce custom implementations for the AWS credentials here to be able to use them as part of a hash key

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.