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

Fix _field_caps serialization in order to support cross cluster search #24722

Merged
merged 4 commits into from
May 17, 2017

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented May 16, 2017

Today the _field_caps API doesn't implement its request serialization
correctly since indices and indices options are not serialized at all.
This will likely break with all transport clients etc. and if this request
must be send across the network. This commit fixes this and adds correct
handling if we have only remote indices to prevent the inclusion of
all local indices.

Today the `_field_caps` API doesn't implement its request serialization
correctly since indices and indices options are not serialized at all.
This will likely break with all transport clients etc. and if this request
must be send across the network. This commit fixes this and adds correct
handling if we have only remote indices to prevent the inclusion of
all local indices.
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 should we also add a unit test for the field caps request serialization?

Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM
Can you also add the missing fields in FieldCapabilitiesRequestTests.

@spalger
Copy link
Contributor

spalger commented May 16, 2017

Works will with elastic/kibana#11114

@s1monw
Copy link
Contributor Author

s1monw commented May 17, 2017

@javanna @jimczi pushed a new commit

Copy link
Contributor

@jimczi jimczi 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 for adding the tests


// change indices options
other.indicesOptions(IndicesOptions.strictExpand());
assertNotEquals(request, other);
Copy link
Member

Choose a reason for hiding this comment

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

maybe you can use EqualsHashCodeTestUtils#checkEqualsAndHashCode and save yourself some boilerplate here?

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.

still LGTM thanks @s1monw left a tiny comment but it doesn't need another review and it can also be addressed later if applicable

@s1monw s1monw merged commit cf846af into elastic:master May 17, 2017
@s1monw s1monw deleted the fix_field_caps_serialization branch May 17, 2017 12:02
s1monw added a commit that referenced this pull request May 17, 2017
…rch (#24722)

Today the `_field_caps` API doesn't implement its request serialization
correctly since indices and indices options are not serialized at all.
This will likely break with all transport clients etc. and if this request
must be send across the network. This commit fixes this and adds correct
handling if we have only remote indices to prevent the inclusion of
all local indices.
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.

6 participants