[ML] Refactor common utils out of ML plugin to XPack.Core #39976

benwtrent · 2019-03-12T19:35:03Z

3 new abstract classes to aid in getting resources.

minor refactors required for such, moved QueryPage, ExpandedIdsMatcher, and PageParams into xpack.core.action.util.

Refactored some of the ML responses. More refactoring will have to be done to rewrite the transports to take advantage of the new abstract class, but did not want this PR to become thousands of lines of changes.

elasticmachine · 2019-03-12T19:35:05Z

Pinging @elastic/ml-core

hendrikmuhs

LGTM, just 1 comment/opinion

hendrikmuhs · 2019-03-12T20:07:55Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/action/util/QueryPage.java

@@ -38,7 +37,7 @@
    public QueryPage(List<T> results, long count, ParseField resultsField) {


devils advocate: as we are moving this into a more prominent place, we should discuss the naming of this, I think QueryPage doesn't fit. It's rather a ResultPage.

Hmm, definitely a possibility. I think the name QueryPage is an artifact of indicating that "This is the results of a Query", but ResultPage does give a broader indication as there is nothing restricting the class.

@dimitris-athanasiou ^^ what say you?

I would even say it is just a Page :-) However, changing this might cause BWC problems, at least on the HLRC and transport level.

Yes, this change is already breaking for transport client users (so I added the >breaking-java label). To move a class from one package to another is not too hard to cope with for a developer with an IDE, as the IDE will find the class of the same name in its new place. But to change both the package and the name of a class in a refactoring is quite unfriendly. Plus if the class is renamed server-side then the HLRC equivalent should be renamed to match, and that means we cause work for HLRC users as well as transport client users (as well as extra work for ourselves changing the HLRC docs). So I am also of the opinion that we should stick with the current name even if it isn't perfect.

I do not see (a) QueryPage being used in HLRC.

I do not see (a) QueryPage being used in HLRC.

Yes, that's true. The HLRC responses just contain Java Lists. So we didn't maintain the interface for response objects between the transport client and the HLRC. I thought we had. Sorry.

True, we've already stepped away from it there with the AbstractResultResponse. I'm glad we did that :-)

dimitris-athanasiou · 2019-03-13T09:08:55Z

@benwtrent I would suggest that this PR has at least one implementation of the transport action. This will also enable testing all that logic. For simplicity, you could start with the get-filters action. It should be straightforward (similar to the implementation of this in the df-analytics branch).

hendrikmuhs · 2019-03-13T10:09:01Z

As others already pointed out, I think this might be a risky change. I therefore bring up the option to introduce the abstractions in core but not to immediately migrate existing ML API's to it. We will use the new abstractions for the new functionality. This gives us time and freedom to stabilize the new abstractions without BWC pain. At a later point we can migrate ML API's after proper testing.

droberts195 · 2019-03-13T11:11:45Z

I don't think it's that risky, although I do think we'll find it's not generic enough in the future.

We have YAML tests that exercise all the REST endpoints that correspond to the refactored actions, so they prove that this change has not broken the existing REST endpoints. I also think if we develop new abstractions with a plan to migrate existing endpoints to them after 7.1 but don't actually do that for 7.1 then it's more likely to mean BWC headaches or never migrating the existing endpoints to the new abstractions.

There are 3 new abstract classes here, for the request, response and transport action. The request and response abstract classes are the simplest and least likely to cause problems. The abstract transport action is the one that's most likely to cause grief in the future. One thing I can see that's missing is for the ExpandedIdsMatcher to have some sort of aliases functionality. For jobs we have job groups, which work like aliases do for indices. But it's not unimaginable in the future that we'd want something similar for some other type of request, maybe transform groups or something. So that makes me think that in the future we'll want ExpandedIdsMatcher to be an interface that the abstract transport action's constructor takes as an argument rather than a concrete class. Then the job transport actions can pass in one that understands how to expand job groups. However, making this change in the future won't affect any public APIs.

I also think that we will find actions where the request and response classes can extend these abstract classes but the transport action cannot, due to having to do something differently. But I don't think that's a problem.

I do agree with @dimitris-athanasiou that it would be best to implement one transport action that extends the abstract transport action just to make sure it is usable in at least one case. But after that I'd be happy to merge this PR.

droberts195 · 2019-03-13T14:02:14Z

...ugin/core/src/main/java/org/elasticsearch/xpack/core/action/AbstractGetResourcesRequest.java

+
+public abstract class AbstractGetResourcesRequest extends ActionRequest {
+
+    public static final String ALLOW_NO_RESOURCES = "allow_no_resources";


I think this is a bit too abstract to be exposed to end users. Where we've exposed this in the past it's been in more targeted parameters like allow_no_jobs. So I think each group of classes that extend this class should define their own parameter rather than the abstract class suggesting one.

droberts195 · 2019-03-13T14:03:49Z

x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/rest/filter/RestGetFiltersAction.java

        }
+        getListRequest.setAllowNoResources(restRequest.paramAsBoolean(ALLOW_NO_RESOURCES, true));


This looks like it has moved beyond refactoring to adding new functionality. I would say that this PR should just hardcode true here, as that's what the behaviour was before. (If we were to have a parameter to say finding no filters was unacceptable then maybe we'd want to call it allow_no_filters.)

droberts195

LGTM

dimitris-athanasiou

LGTM

benwtrent · 2019-03-13T17:37:21Z

run elasticsearch-ci/1

) * [ML] Refactor common utils out of ML plugin to XPack.Core * implementing GET filters with abstract transport * removing added rest param * adjusting how defaults can be supplied

…40009) * [ML] Refactor common utils out of ML plugin to XPack.Core * implementing GET filters with abstract transport * removing added rest param * adjusting how defaults can be supplied

benwtrent · 2019-03-14T15:03:21Z

@jasontedor ping This PR added some commonly useful code to xpack.core.action

When get filters is called without setting the `size` paramter only up to 10 filters are returned. However, 100 filters should be returned. This commit fixes this and adds an integ test to guard it. It seems this was accidentally broken in elastic#39976. Closes elastic#54206

When get filters is called without setting the `size` paramter only up to 10 filters are returned. However, 100 filters should be returned. This commit fixes this and adds an integ test to guard it. It seems this was accidentally broken in #39976. Closes #54206

When get filters is called without setting the `size` paramter only up to 10 filters are returned. However, 100 filters should be returned. This commit fixes this and adds an integ test to guard it. It seems this was accidentally broken in elastic#39976. Closes elastic#54206 Backport of elastic#54207

When get filters is called without setting the `size` paramter only up to 10 filters are returned. However, 100 filters should be returned. This commit fixes this and adds an integ test to guard it. It seems this was accidentally broken in #39976. Closes #54206 Backport of #54207

[ML] Refactor common utils out of ML plugin to XPack.Core

2f045c7

benwtrent added >refactoring :ml Machine learning v8.0.0 v7.2.0 labels Mar 12, 2019

hendrikmuhs approved these changes Mar 12, 2019

View reviewed changes

benwtrent mentioned this pull request Mar 12, 2019

[Data Frame] Refactor PUT transform to not create a task #39934

Merged

droberts195 added the >breaking-java label Mar 13, 2019

implementing GET filters with abstract transport

c21ddee

droberts195 reviewed Mar 13, 2019

View reviewed changes

removing added rest param

af29942

droberts195 approved these changes Mar 13, 2019

View reviewed changes

dimitris-athanasiou approved these changes Mar 13, 2019

View reviewed changes

adjusting how defaults can be supplied

bd14570

benwtrent merged commit b0d7782 into elastic:master Mar 13, 2019

benwtrent deleted the feature/x-pack-provide-abstract-resource-action-transport branch March 13, 2019 19:23

dimitris-athanasiou mentioned this pull request Mar 25, 2020

[ML] Get ML filters size should default to 100 #54207

Merged

dimitris-athanasiou mentioned this pull request Mar 26, 2020

[7.x][ML] Get ML filters size should default to 100 (#54207) #54278

Merged

dimitris-athanasiou mentioned this pull request Mar 26, 2020

[7.7][ML] Get ML filters size should default to 100 (#54207) #54279

Merged

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 3) elastic/elasticsearch-net#4534

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Refactor common utils out of ML plugin to XPack.Core #39976

[ML] Refactor common utils out of ML plugin to XPack.Core #39976

benwtrent commented Mar 12, 2019

elasticmachine commented Mar 12, 2019

hendrikmuhs left a comment

hendrikmuhs Mar 12, 2019

benwtrent Mar 12, 2019

benwtrent Mar 12, 2019

dimitris-athanasiou Mar 13, 2019

droberts195 Mar 13, 2019

hendrikmuhs Mar 13, 2019

droberts195 Mar 13, 2019

dimitris-athanasiou Mar 13, 2019

dimitris-athanasiou commented Mar 13, 2019

hendrikmuhs commented Mar 13, 2019

droberts195 commented Mar 13, 2019

droberts195 Mar 13, 2019

droberts195 Mar 13, 2019

droberts195 left a comment

dimitris-athanasiou left a comment

benwtrent commented Mar 13, 2019

benwtrent commented Mar 14, 2019

		@@ -38,7 +37,7 @@
		public QueryPage(List<T> results, long count, ParseField resultsField) {


		public abstract class AbstractGetResourcesRequest extends ActionRequest {

		public static final String ALLOW_NO_RESOURCES = "allow_no_resources";

		}
		getListRequest.setAllowNoResources(restRequest.paramAsBoolean(ALLOW_NO_RESOURCES, true));

[ML] Refactor common utils out of ML plugin to XPack.Core #39976

[ML] Refactor common utils out of ML plugin to XPack.Core #39976

Conversation

benwtrent commented Mar 12, 2019

elasticmachine commented Mar 12, 2019

hendrikmuhs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimitris-athanasiou commented Mar 13, 2019

hendrikmuhs commented Mar 13, 2019

droberts195 commented Mar 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droberts195 left a comment

Choose a reason for hiding this comment

dimitris-athanasiou left a comment

Choose a reason for hiding this comment

benwtrent commented Mar 13, 2019

benwtrent commented Mar 14, 2019