[BEAM-2541] Check Elasticsearch backend version when the pipeline is run not when it is constructed by echauchot · Pull Request #3493 · apache/beam

echauchot · 2017-07-05T10:21:58Z

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify.
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

coveralls · 2017-07-05T11:52:16Z

Coverage decreased (-0.02%) to 70.829% when pulling f64156c on echauchot:BEAM-2541-move-ES-version-check-to-validate into b8ac326 on apache:master.

peihe

Is that possible to add a unit test (maybe by mocking or faking RestClient)?

peihe · 2017-07-17T06:49:50Z

...ava/io/elasticsearch/src/main/java/org/apache/beam/sdk/io/elasticsearch/ElasticsearchIO.java

 @Experimental(Experimental.Kind.SOURCE_SINK)
 public class ElasticsearchIO {

+  private static void checkVersion(ConnectionConfiguration connectionConfiguration)


Move it back to keep the history (And, I think private methods are not that important to be on the top.)

Previously checkVersion was in ConnectionConfiguration class. I cannot move it back to that class because otherwise version check would be done at configuration time and all the point of this PR is to do the check at run time and not at configuration time. But I agree, with the other part of your comment: private method needs to be at the bottom, I'll move the method at the bottom of ElasticsearchIO class

peihe · 2017-07-17T06:56:01Z

...ava/io/elasticsearch/src/main/java/org/apache/beam/sdk/io/elasticsearch/ElasticsearchIO.java

    @Override
    public void validate(PipelineOptions options) {
+      ConnectionConfiguration connectionConfiguration = getConnectionConfiguration();
      checkState(


update to checkNotNull (since you are here)

-1. There was a discussion about that in the original PR (#1439) and also in https://issues.apache.org/jira/browse/BEAM-959 (see also related PR). CheckNotNull does not generate an explicit enough message to users in that case. And besides the javadoc of checkNotNull specifies "Ensures that an object reference passed as a parameter to the calling method is not null". It is not a parameter in that case.

+1 with @echauchot: checkArgument() and checkState() should be always preferred.

peihe · 2017-07-17T06:56:33Z

...ava/io/elasticsearch/src/main/java/org/apache/beam/sdk/io/elasticsearch/ElasticsearchIO.java

    @Override
    public void validate(PipelineOptions options) {
+      ConnectionConfiguration connectionConfiguration = getConnectionConfiguration();
      checkState(


update to checkNotNull (since you are here)

peihe · 2017-07-17T06:57:31Z

...ava/io/elasticsearch/src/main/java/org/apache/beam/sdk/io/elasticsearch/ElasticsearchIO.java

+          connectionConfiguration != null,
          "ElasticsearchIO.write() requires a connection configuration"
              + " to be set via withConnectionConfiguration(configuration)");
+      try {


catch checkVersion and re-throw in checkVersion.

Totally agree, thanks.

peihe · 2017-07-17T07:02:56Z

R: +@peihe

echauchot · 2017-07-20T14:53:55Z

@peihe, thanks for your review.
I'll mock the RestClient to test checkVersion (mainly to verify that the exception is thrown while running the pipeline and not while configuring it) and ping you for another look.

coveralls · 2017-07-21T11:27:22Z

Coverage decreased (-0.01%) to 70.599% when pulling 8a3acfb on echauchot:BEAM-2541-move-ES-version-check-to-validate into 1d9160f on apache:master.

echauchot · 2017-07-21T12:08:20Z

I pushed another commit for the unit testing checkVersion using mock that you required. But I find it is a weak test because in the end, it just tests json parsing and exception throwing as we cannot use mocks in a real pipeline because mocks are not serializable. @peihe WDYT?

coveralls · 2017-07-21T13:28:28Z

Coverage decreased (-0.003%) to 70.606% when pulling 186115c on echauchot:BEAM-2541-move-ES-version-check-to-validate into 1d9160f on apache:master.

peihe · 2017-07-24T08:19:45Z

I agree mocking the response string in the unit tests doesn't have much value, and the string is hard to understand and maintain. So, I would prefer to revert the last commit.
The rest changes looks good to me.

jbonofre · 2017-07-24T08:23:10Z

Agree to revert the last commit.

echauchot · 2017-07-24T09:17:55Z

We all agree that the UT of checkVersion is very weak, I'll remove the last commit

coveralls · 2017-07-24T10:50:21Z

Coverage increased (+0.01%) to 70.596% when pulling 8a3acfb on echauchot:BEAM-2541-move-ES-version-check-to-validate into f54072a on apache:master.

coveralls · 2017-07-26T11:47:17Z

Coverage decreased (-0.2%) to 70.404% when pulling 5bbbbfd on echauchot:BEAM-2541-move-ES-version-check-to-validate into f54072a on apache:master.

echauchot · 2017-07-26T12:25:05Z

@jbonofre, I pushed a cleaning commit, PTAL.

…run not when it is constructed

IOException is never thrown in ConnectionConfiguration.create() final field try with resources

jbonofre

LGTM. I'm waiting the end of Jenkins before merging. Thanks !

coveralls · 2017-07-27T16:48:30Z

Coverage remained the same at 70.471% when pulling 5ca988a on echauchot:BEAM-2541-move-ES-version-check-to-validate into 892a2cf on apache:master.

peihe reviewed Jul 17, 2017

View reviewed changes

echauchot force-pushed the BEAM-2541-move-ES-version-check-to-validate branch from f64156c to 8a3acfb Compare July 21, 2017 10:09

echauchot force-pushed the BEAM-2541-move-ES-version-check-to-validate branch from 186115c to 8a3acfb Compare July 24, 2017 09:22

echauchot added 4 commits July 27, 2017 17:23

[BEAM-2541] Check Elasticsearch backend version when the pipeline is …

6195cf4

…run not when it is constructed

Move checkVersion to the bottom of ElasticsearchIO class

0618119

Catch IOException in checkVersion not outside

fa5e25c

Cleaning

5ca988a

IOException is never thrown in ConnectionConfiguration.create() final field try with resources

echauchot force-pushed the BEAM-2541-move-ES-version-check-to-validate branch from 5bbbbfd to 5ca988a Compare July 27, 2017 15:25

jbonofre approved these changes Jul 27, 2017

View reviewed changes

asfgit closed this in 6ca5305 Jul 28, 2017

echauchot deleted the BEAM-2541-move-ES-version-check-to-validate branch July 28, 2017 12:57

Conversation

echauchot commented Jul 5, 2017

Uh oh!

coveralls commented Jul 5, 2017

Uh oh!

peihe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

echauchot Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peihe commented Jul 17, 2017

Uh oh!

echauchot commented Jul 20, 2017

Uh oh!

coveralls commented Jul 21, 2017

Uh oh!

echauchot commented Jul 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 21, 2017

Uh oh!

peihe commented Jul 24, 2017

Uh oh!

jbonofre commented Jul 24, 2017

Uh oh!

echauchot commented Jul 24, 2017

Uh oh!

coveralls commented Jul 24, 2017

Uh oh!

coveralls commented Jul 26, 2017

Uh oh!

echauchot commented Jul 26, 2017

Uh oh!

jbonofre left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

echauchot Jul 19, 2017 •

edited

Loading

echauchot commented Jul 21, 2017 •

edited

Loading