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

Clarify some ToXContent implementations behaviour #41000

Merged
merged 6 commits into from Apr 15, 2019

Conversation

cbuescher
Copy link
Member

This change adds either ToXContentObject or ToXContentFragment to classes
directly implementing ToXContent currently. This helps in reasoning about
whether those implementations output full xcontent object or just fragments.

Relates to #16347

This change adds either ToXContentObject or ToXContentFragment to classes
directly implementing ToXContent currently. This helps in reasoning about
whether those implementations output full xcontent object or just fragments.

Relates to elastic#16347
@cbuescher cbuescher added >enhancement :Core/Infra/Core Core issues without another label labels Apr 9, 2019
@cbuescher cbuescher requested a review from javanna April 9, 2019 09:30
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

thanks @cbuescher for picking this up. I left a couple of comments, especially on making requests and response implement ToXContentObject, if that makes sense.

@@ -150,7 +152,7 @@ public boolean higherThan(Type other) {
/**
* Simple class representing a single decision
*/
public static class Single extends Decision {
public static class Single extends Decision implements ToXContentObject {
Copy link
Member

Choose a reason for hiding this comment

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

These two are confusing me. Given that the Decision is included in other objects, and it can either be an object or a fragment, I am not following how it can work, given that where we include them we simply call toXContent against them. Am I wrong thinking that only one of the two implementations can work and the other one may break the json?

Copy link
Member Author

Choose a reason for hiding this comment

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

To me it looks like the Single decisions are self-contained json, whereas the Multi decision is composed of several single decisions and just prints those in consecutive order. This seems to work because we alway seem to wrap them in an array (e.g. in MoveDecision#toXContent, NodeAllocationResult#toXContent). I think this is why the parent Decision is solely implementing ToXContent because the only thing guaranteed is that you can somehow deserialize it. The object hierarchy might be a bit off here but I think its out of scope for this PR to fix this.

@cbuescher
Copy link
Member Author

@elasticmachine run elasticsearch-ci/packaging-sample

@cbuescher
Copy link
Member Author

cbuescher commented Apr 12, 2019

@javanna thanks for the review, I hope I adressed most of your concerns, left a reply about your question about the "Decision" objects. I'm not exactly sure how to proceed here or if we should just leave it as-is for the moment. In any case, I think this is ready for another look.

Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM thanks @cbuescher

@cbuescher cbuescher merged commit cb3b213 into elastic:master Apr 15, 2019
cbuescher pushed a commit that referenced this pull request Apr 15, 2019
This change adds either ToXContentObject or ToXContentFragment to classes
directly implementing ToXContent currently. This helps in reasoning about
whether those implementations output full xcontent object or just fragments.

Relates to #16347
@cbuescher
Copy link
Member Author

@javanna thanks for the review

gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
This change adds either ToXContentObject or ToXContentFragment to classes
directly implementing ToXContent currently. This helps in reasoning about
whether those implementations output full xcontent object or just fragments.

Relates to elastic#16347
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.

None yet

4 participants