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

[Transform]rename classes in transform plugin #46784

Merged

Conversation

hendrikmuhs
Copy link

@hendrikmuhs hendrikmuhs commented Sep 17, 2019

rename classes in transform plugin

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

@hendrikmuhs hendrikmuhs force-pushed the rename-to-transform-plug-classes branch from 9d203e3 to eef09a1 Compare September 18, 2019 09:45
@hendrikmuhs hendrikmuhs marked this pull request as ready for review September 19, 2019 09:28
@hendrikmuhs hendrikmuhs removed the WIP label Sep 19, 2019
Copy link
Contributor

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

LGTM, but I realised it's not very hard to provide BWC for the "enabled" setting, so I left comments on how to do it. Let me know what you think.

@@ -41,7 +41,7 @@ private XPackSettings() {
public static final Setting<Boolean> CCR_ENABLED_SETTING = Setting.boolSetting("xpack.ccr.enabled", true, Property.NodeScope);

/** Setting for enabling or disabling data frame. Defaults to true. */
public static final Setting<Boolean> DATA_FRAME_ENABLED = Setting.boolSetting("xpack.data_frame.enabled", true,
public static final Setting<Boolean> TRANSFORM_ENABLED = Setting.boolSetting("xpack.transform.enabled", true,
Setting.Property.NodeScope);
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we said we wouldn't provide BWC for this, but it's actually trivial to do it and it might make someone's life easier:

    // TODO remove before 8.0.0 release
    private static final Setting<Boolean> DATA_FRAME_ENABLED = Setting.boolSetting("xpack.data_frame.enabled", true,
            Setting.Property.NodeScope, Setting.Property.Deprecated);
    public static final Setting<Boolean> TRANSFORM_ENABLED = Setting.boolSetting("xpack.transform.enabled", DATA_FRAME_ENABLED,
            Setting.Property.NodeScope);

The Setting class completely takes care of the deprecation warnings.

So maybe we should?

We'll have other tweaks we need to make before 8.0.0 release, such as the role names, so it's not even like this creates much work for us.

@@ -217,7 +217,7 @@ private XPackSettings() {
settings.add(PASSWORD_HASHING_ALGORITHM);
settings.add(INDEX_LIFECYCLE_ENABLED);
settings.add(SNAPSHOT_LIFECYCLE_ENABLED);
settings.add(DATA_FRAME_ENABLED);
Copy link
Contributor

Choose a reason for hiding this comment

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

If you agree it's not too hard to provide BWC for this then this line should stay with a TODO to remove it before 8.0.0 release.

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.

4 participants