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

Renames and refactoring for reloadable plugins #30992

Conversation

albertzaharovits
Copy link
Contributor

@albertzaharovits albertzaharovits commented May 31, 2018

This handles some renames and javadocs for the reload-secure-store-action feature branch to deck it before merging to master. This is required because the scope of the branch was somewhat fluid, and as the work progressed, the names and comments were not illustrative for their purposes. This has mostly ornamental value but it does contains two refactorings, hence the encompassing PR title.

Here is the list with all renames:

NodesReInitRequestBuilder -> NodesReloadSecureSettingsRequestBuilder
NodesReInitRequest -> NodesReloadSecureSettingsRequest
NodesReInitResponse -> NodesReloadSecureSettingsResponse
NodesReInitAction -> NodesReloadSecureSettingsAction
"cluster:admin/reinit" -> "cluster:admin/nodes/reload_secure_settings"
secureStorePassword -> secureSettingsPassword
TransportNodesReInitAction -> TransportNodesReloadSecureSettingsAction
package org.elasticsearch.action.admin.cluster.reinit; -> package org.elasticsearch.action.admin.cluster.node.reload;
ReInitPlugin -> ReloadablePlugin
ReInitPlugin#reinit -> ReloadablePlugin#reload
RestReInitAction -> RestReloadSecureSettingsAction
AbstractClient#prepareReInit -> AbstractClient#prepareReloadSecureSettings
request.param("secureStorePassword" -> request.param("secure_seettings_password"
#updateClientSettings -> #refreshAndClearCache

All services that lend clients (AwsS3Service, AwsEc2Service, GoogleCloudStorageService and AzureStorageService) have the methods client and refreshAndClearCache. This client method creates (and cache) and returns a client (or a releasable wrapper). The refreshAndClearCache method updates the settings from which new clients are built and clears the cache of the old clients.

Besides renames, there is one notable refactoring: ReloadablePlugin#reload now returns an exception on failure instead of a bland false return value. This cascades back to the node response and how it deals with error handling.
The other refactoring is that the method releaseCachedClient (for AwsS3Service and AwsEc2Service is now hidden and exposed through Closeable#close).

Relates #29135

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@albertzaharovits albertzaharovits changed the title Renames and refactoring Renames and refactoring for the reloadable plugins May 31, 2018
@albertzaharovits albertzaharovits changed the title Renames and refactoring for the reloadable plugins Renames and refactoring for reloadable plugins May 31, 2018
Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

Not much to say, I looked at the renamings and it LGTM.

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

LGTM. I left a comment for something to handle in a followup.

* of the node's keystore (backing the implementation of
* {@code SecureSettings}).
*/
private String secureSettingsPassword;
Copy link
Member

Choose a reason for hiding this comment

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

why don't we use a SecureString here? I know this is not anything new in this PR so we can handle in a follow up.

@albertzaharovits albertzaharovits merged commit c4ebd6f into elastic:reload-secure-store-action Jun 5, 2018
@albertzaharovits albertzaharovits deleted the renames-and-refactoring branch June 5, 2018 08:43
@jimczi jimczi 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants