Skip to content

Conversation

@henningandersen
Copy link
Contributor

@henningandersen henningandersen commented Nov 23, 2020

Named writeables were not registered.

This looks like it was a backport failure when backporting #59309 in #65151, since this is the first named writable registered by the ml module in 7.x.

Named writeables were not registered.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (:ml)

@dimitris-athanasiou
Copy link
Contributor

This was dropped during the backport indeed. But the reason is different. In 7.x, we need to register named writeables via XPackClientPlugin.getNamedWriteables() in order to keep working with the transport client. The latter is removed in master so we have refactored the way we register named writeables there in #53757. However, this discrepancy between master and 7.x can cause backport issues like this one. For consistency, could you register the autoscaling writeables in XPackClientPlugin in 7.x?

@henningandersen
Copy link
Contributor Author

Given that autoscaling is still experimental, I think we should not move things to xpack-core ahead of time. I have put it on our autoscaling sync for next week to discuss this. I would be inclined to just document that autoscaling does not work with the transport client.

Moving the specific ML class would require moving other parts of autoscaling to xpack-core too. And for it to be functional, actions, requests and responses have to move too.

So for now, I would prefer to get this PR in as is to enable consumers of the API to start testing ML autoscaling and then we can tackle the move to xpack core (if that is the decision taken) in a follow-up. Would that be ok?

Copy link
Contributor

@dimitris-athanasiou dimitris-athanasiou left a comment

Choose a reason for hiding this comment

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

LGTM Your point totally makes sense.

@henningandersen
Copy link
Contributor Author

Thanks Dimitris!

@henningandersen henningandersen merged commit 64e3209 into elastic:7.x Nov 24, 2020
henningandersen added a commit that referenced this pull request Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants