Skip to content

[BEAM-3112] Improve error messages in ElasticsearchIO test utils#4051

Closed
echauchot wants to merge 1 commit intoapache:masterfrom
echauchot:BEAM-3112-improve-es-test-utils-logs
Closed

[BEAM-3112] Improve error messages in ElasticsearchIO test utils#4051
echauchot wants to merge 1 commit intoapache:masterfrom
echauchot:BEAM-3112-improve-es-test-utils-logs

Conversation

@echauchot
Copy link
Contributor

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

R: @iemejia

@jbonofre
Copy link
Member

R: @jbonofre

@iemejia
Copy link
Member

iemejia commented Oct 30, 2017

retest this please

Copy link
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for jenkins to merge.
If you think it is worth the effort maybe it is a good idea to add a test method for checkForErrors to validate its resiliance in case of missing fields.

Copy link
Member

@jbonofre jbonofre left a comment

Choose a reason for hiding this comment

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

Lgtm

@iemejia
Copy link
Member

iemejia commented Oct 30, 2017

Minor minor comment, don't forget to add an extra space before curly braces (I will fix this on merge).
See point 3 on https://google.github.io/styleguide/javaguide.html#s4.6.2-horizontal-whitespace

@echauchot
Copy link
Contributor Author

Thanks for the review guys!
@iemejia
1.checkForErrors is already tested in ElasticsearchIOTest.testWriteWithErrors which tries to insert 2 malformed docs. I just added a call to this method in ElasticsearchTestUtils.insertTestDocuments
2. about the curly braces, I thought about adding this rule to checktyle.xml

@echauchot
Copy link
Contributor Author

retest this please

@asfgit asfgit closed this in 16b9d58 Oct 30, 2017
@iemejia
Copy link
Member

iemejia commented Oct 30, 2017

@echauchot It is a good idea to fix the braces issue on the checkstyle if you find a way to enforce it (I am surprised it is not enforced there), from a quicklook however it seems you will need to fix ~70 classes that don't respect the braces style.

@echauchot echauchot deleted the BEAM-3112-improve-es-test-utils-logs branch October 31, 2017 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants