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

[FLINK-7350] [travis] Only execute japicmp in misc profile #4461

Closed
wants to merge 3 commits into from

Conversation

zentol
Copy link
Contributor

@zentol zentol commented Aug 2, 2017

Based on #4460.

What is the purpose of the change

Improve travis build times and stability by skipping redundant japicmp executions.

Note that the japicmp 0.7 does not allow skipping the execution form the command-line (only added in 0.10), so I worked around that with a profile that is activated by specifying -Djapicmp.skip=true, which is how it would work in 0.10.

Brief change log

  • add japicmp.skip property to skip execution
  • add skip-japicmp profile to skip execution, activated by specifying "-Djapicmp.skip=true"
  • add -Djapicmp.skip=true to core/libraries/connector/tests profile options

Verifying this change

Remove the @Public annotation from any class and run it on travis, should only fail 1 profile (i.e. 2 builds)

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@greghogan
Copy link
Contributor

Should we upgrade to japicmp 0.10 or delay this change until such time?

@zentol
Copy link
Contributor Author

zentol commented Aug 2, 2017

I tried upgrading the plugin, but then more violations were detected causing the build to fail. I didn't have the time to properly investigate whether it makes sense to upgrade the plugin now or not.

The nice thing is that if we ever decide to upgrade it later on we only have to remove the profile that i add, the travis scripts will work as is.

Copy link
Contributor

@greghogan greghogan left a comment

Choose a reason for hiding this comment

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

What do you think of adding a note to the skip-japicmp profile documenting that this can be removed when upgrading to 0.10+?

MVN_COMPILE_OPTIONS="$MVN_COMPILE_OPTIONS -DskipTests"

MVN_COMPILE="mvn $MVN_COMMON_OPTIONS $MVN_COMPILE_OPTIONS $PROFILE $MVN_COMPILE_MODULES clean install"
MVN_TEST="mvn -$MVN_COMMON_OPTIONS $PROFILE $MVN_TEST_MODULES verify"
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks to be a spurious - after mvn.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it in #4460.

@zentol
Copy link
Contributor Author

zentol commented Aug 2, 2017

@greghogan That's a great idea, implemented it.

@zentol
Copy link
Contributor Author

zentol commented Aug 7, 2017

merging.

zentol added a commit to zentol/flink that referenced this pull request Aug 7, 2017
@asfgit asfgit closed this in be8eb1a Aug 7, 2017
@zentol zentol deleted the 7350 branch August 7, 2017 13:21
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.

3 participants