Skip to content

Conversation

@martijnvg
Copy link
Member

Relates to #24939

@martijnvg martijnvg added review >test Issues or PRs that are addressing/adding tests v6.0.0 labels May 30, 2017
@martijnvg martijnvg requested a review from nik9000 May 30, 2017 09:16
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

I left a few suggestions for improvement or followups.

Can you remove the OldIndexBackwardsCompatibilityIT#assertBasicSearchWorks method so we're sure we've ported it?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need the Integer.toString bit.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe Map<String, Object> toMap(Response response)?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe testSearch instead?

I think it'd be nice to move some of the random document generation from testRandomDocumentsAndSnapshot below into this and just use testRandomDocumentsAndSnapshot for testing the translog stuff.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it should just be long instead of long_sort.

@martijnvg
Copy link
Member Author

@nik9000 I've updated the PR, based on your feedback.

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Left a minor but otherwise LGTM.

Copy link
Member

Choose a reason for hiding this comment

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

We can allow flush here, I think.

@martijnvg martijnvg force-pushed the move_OldIndexBackwardsCompatibilityIT_to_qa_module_part_1 branch from bca98cb to 0d1a487 Compare May 30, 2017 15:59
@martijnvg martijnvg force-pushed the move_OldIndexBackwardsCompatibilityIT_to_qa_module_part_1 branch from 0d1a487 to 9531ef2 Compare May 31, 2017 07:28
@martijnvg martijnvg merged commit 9531ef2 into elastic:master May 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>test Issues or PRs that are addressing/adding tests v6.0.0-alpha2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants