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-DataFrame] fix wire serialization issues in data frame response objects #39790

Merged

Conversation

hendrikmuhs
Copy link

fixes wire serialization issues in data frame response objects uncovered on a multi node setup

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

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

One more thing that falls into the category of serialization problems but could be left to a followup PR (or could be added to this PR) is that all action Request classes in this plugin have public constructors that take no arguments and construct an object that violates the invariants expected by serialization. They're only called by the RequestBuilders, so I think these Request() constructors should be removed and the constructors of the RequestBuilders should be changed to force the user to pass the minimum arguments required to make the Requests valid. But, like I said, this can be done in a separate PR if you prefer.

@hendrikmuhs
Copy link
Author

@droberts195

It's not as easy at it looks, I decreased accessibility where it was straight-forward. For other cases I run into problems. The code seems to follow common patterns, I see the same "issue" elsewhere - which does not mean it shouldn't be fixed - however it requires further thought.

I am always open for simplifications, it pays off in the long run. So the rest can be fixed in a follow-up PR (e.g. I am unsure if we really need all the RequestBuilder - they seem unused).

@hendrikmuhs hendrikmuhs merged commit 4cab8ec into elastic:master Mar 7, 2019
hendrikmuhs pushed a commit that referenced this pull request Mar 7, 2019
…bjects (#39790)

fix wire serialization issues in data frame response objects
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 8, 2019
* elastic/master:
  Add pre-upgrade check to test cluster routing allocation is enabled (elastic#39340)
  Update logstash-management.json to use typeless template (elastic#38653)
  Small simplifications to mapping validation. (elastic#39777)
  Update distribution build instructions to reflect file names with OS/architecture classifiers. (elastic#39762)
  Give jspawnhelper execute permissions in bundled JDK (elastic#39787)
  Maintain step order for ILM trace logging (elastic#39522)
  [ML-DataFrame] fix wire serialization issues in data frame response objects (elastic#39790)
  fix index refresh in test within 20_mix_typeless_typeful (elastic#39198)
  Combine overriddenOps and skippedOps in translog (elastic#39771)
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