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.

Sign up for GitHub

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

Do not add index event listener if CCR disabled #37432

Merged
merged 12 commits into from
Jan 18, 2019
Merged

Do not add index event listener if CCR disabled #37432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
78ccb3c
Do not add index event listener if CCR disabled
Tim-Brooks Jan 14, 2019
3c91938
Add test
Tim-Brooks Jan 14, 2019
976eeeb
Work on changes
Tim-Brooks Jan 14, 2019
553b46b
Merge remote-tracking branch 'upstream/master' into do_no_add_listener
Tim-Brooks Jan 15, 2019
b4fedc8
Add todo
Tim-Brooks Jan 15, 2019
e10820e
Fix checkstyle
Tim-Brooks Jan 15, 2019
c936b84
Merge remote-tracking branch 'upstream/master' into do_no_add_listener
Tim-Brooks Jan 15, 2019
bf5a195
Merge remote-tracking branch 'upstream/master' into do_no_add_listener
Tim-Brooks Jan 16, 2019
2c7192c
Merge remote-tracking branch 'upstream/master' into do_no_add_listener
Tim-Brooks Jan 16, 2019
c4a89c9
Merge remote-tracking branch 'upstream/master' into do_no_add_listener
Tim-Brooks Jan 16, 2019
b8239cd
Merge remote-tracking branch 'upstream/master' into do_no_add_listener
Tim-Brooks Jan 16, 2019
db518d5
Merge remote-tracking branch 'upstream/master' into do_no_add_listener
Tim-Brooks Jan 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/Ccr.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,9 @@ public Map<String, Repository.Factory> getInternalRepositories(Environment env,

@Override
public void onIndexModule(IndexModule indexModule) {
indexModule.addIndexEventListener(this.restoreSourceService.get());
if (enabled) {
indexModule.addIndexEventListener(this.restoreSourceService.get());
}
}

protected XPackLicenseState getLicenseState() { return XPackPlugin.getSharedLicenseState(); }
Expand Down
45 changes: 45 additions & 0 deletions x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/CcrDisabledIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

package org.elasticsearch.xpack.ccr;

import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.test.ESIntegTestCase;
import org.elasticsearch.xpack.core.XPackClientPlugin;
import org.elasticsearch.xpack.core.XPackSettings;

import java.util.Collection;
import java.util.Collections;

public class CcrDisabledIT extends ESIntegTestCase {

public void testClusterCanStartWithCcrInstalledButNotEnabled() throws Exception {
// TODO: Assert that x-pack ccr feature is not enabled once feature functionality has been added
ensureGreen();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the problem that is fixed here only occurs when an index is created, so let's create an index in this test. Perhaps also invoke the XPackClient.prepareInfo() API to check whether the plugin is disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean I specifically tried and the test failed without the my fix in the Ccr.java class. You can see it happens when the add listener method is called:

    public void addIndexEventListener(IndexEventListener listener) {
        ensureNotFrozen();
        if (listener == null) {
            throw new IllegalArgumentException("listener must not be null");
        }
        if (indexEventListeners.contains(listener)) {
            throw new IllegalArgumentException("listener already added");
        }

        this.indexEventListeners.add(listener);
    }

I will add the prepareInfo check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also - it looks like CCR has not implemented the XPackFeatureSet functionality? When I search for references, it is not referenced in the ccr package.

Copy link
Member

Choose a reason for hiding this comment

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

There is a PR open for this.

}

@Override
protected Settings nodeSettings(int nodeOrdinal) {
return Settings.builder().put(super.nodeSettings(nodeOrdinal)).put(XPackSettings.CCR_ENABLED_SETTING.getKey(), true)
.put(XPackSettings.SECURITY_ENABLED.getKey(), false).build();
}

@Override
protected Settings transportClientSettings() {
return Settings.builder().put(super.transportClientSettings()).put(XPackSettings.SECURITY_ENABLED.getKey(), false).build();
}

@Override
protected Collection<Class<? extends Plugin>> nodePlugins() {
return Collections.singletonList(LocalStateCcr.class);
}

@Override
protected Collection<Class<? extends Plugin>> transportClientPlugins() {
return Collections.singletonList(XPackClientPlugin.class);
}
}