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

[ML] Use registry for transform templates #63024

Closed
wants to merge 5 commits into from

Conversation

davidkyle
Copy link
Member

@davidkyle davidkyle commented Sep 29, 2020

Change the way transform templates are installed to use a IndexTemplateRegistry with mappings stored in resource files.

This is the typical way of doing things for many plugins and is the same approach used by ml. This change will enable some common code changes to the auditor in transforms and ml.

assertThat(audit.settings().size(), equalTo(3));
}

public void testRefactoredMappingsAreSameAsOld() throws IOException {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is key because it proves the mappings have not been altered by the refactoring. The above test asserts none of the other settings have changed.

The mappings defined in TransformInternalIndex are used in this test and no longer required once I've proved the mappings have not changed. Once the build is green and the PR approved I'd like to remove the static mapping definitions in TransformInternalIndex and this test.

@davidkyle davidkyle marked this pull request as ready for review September 30, 2020 11:44
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml/Transform)

@davidkyle
Copy link
Member Author

The build failure is from TransformAuditorIT.testAliasCreatedforBWCIndexes

@hendrikmuhs I cannot see how this change would break that test, could you take a look please

java.lang.AssertionError
	at __randomizedtesting.SeedInfo.seed([9EAF9B8CC387C078:CC370CA406B841BF]:0)
	at org.junit.Assert.fail(Assert.java:86)
	at org.junit.Assert.assertTrue(Assert.java:41)
	at org.junit.Assert.assertTrue(Assert.java:52)
	at org.elasticsearch.xpack.transform.integration.TransformAuditorIT.lambda$testAliasCreatedforBWCIndexes$2(TransformAuditorIT.java:110)
	at org.elasticsearch.test.ESTestCase.assertBusy(ESTestCase.java:934)
	at org.elasticsearch.test.ESTestCase.assertBusy(ESTestCase.java:907)
	at org.elasticsearch.xpack.transform.integration.TransformAuditorIT.testAliasCreatedforBWCIndexes(TransformAuditorIT.java:109)

@davidkyle
Copy link
Member Author

The test failure is fixed by #63162

@davidkyle
Copy link
Member Author

Tests have shown that the use of the template registry rather than the Plugin::getIndexTemplateMetadataUpgrader() method causes some problems because the registry updates the templates sometime after the master node comes up.
In the case of rolling upgrades this is not soon enough.

We could add logic to wait for the templates to be installed but the extra complications and hard to maintain code are not worth the small simplification using the registry would enable.

Closing without merging

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