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-3129] Add tooling to ensure interface stability #2042

Closed
wants to merge 3 commits into from

Conversation

rmetzger
Copy link
Contributor

@rmetzger rmetzger commented May 27, 2016

This pull request adds a maven module for ensuring interface stability (japicmp).

I needed to revert some of the changes since 1.0:

  • I made the getMetricGroup() in the RuntimeContext an evolving API.
  • I added two deprecated config constants again.
  • I reintroduced the deprecated Key interface.

Once this has been merged, I'll file JIRAs for 2.0 to remove these changes.

@mxm
Copy link
Contributor

mxm commented May 27, 2016

Thanks for the pull request! Could you elaborate on how and where this will be integrated with our CI or release process?

@rmetzger
Copy link
Contributor Author

The plugin runs with every mvn verify call.

For each maven module, it will download the last stable release (in this case 1.0.0) and check it against the build (currently the 1.1-SNAPSHOT).

@mxm
Copy link
Contributor

mxm commented May 27, 2016

+1 Looks good to me.

Only wondering, how much will it affect our build time? We might want to run this in a dedicated profile.

@rmetzger
Copy link
Contributor Author

Thank you for the review.

Its very low overhead since the 1.0.0 jar's are cached in the local .m2 directory and the tool is doing the check really fast.

@@ -82,6 +82,10 @@

private static final long DEFAULT_RESTART_DELAY = 10000L;

// This field was used as a key for storing the EC in the Job Configuration
@Deprecated
public static final String CONFIG_KEY = "runtime.config";
Copy link
Contributor

Choose a reason for hiding this comment

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

this change belongs in the "Fix breaking changes in flink-core" commit, unless they will be squashed.

@asfgit asfgit closed this in 6c07936 May 27, 2016
@rmetzger
Copy link
Contributor Author

@zentol I think your concern was addressed here: c20efc6

Thanks all for the review!

@rmetzger rmetzger deleted the flink3129-third branch May 27, 2016 16:00
@zentol
Copy link
Contributor

zentol commented May 27, 2016

no my concern wasn't even remotely addressed.
The public static String CONFIG_KEY was removed in commit 5a7f4e3, violating the interface stability.

In commit 6c07936 you add CONFIG_KEY in the ExecutionConfig, while the subsequent commit b0acd97 removes it again.

so now the public static key is still removed which shouldn't be the case, no?

@rmetzger
Copy link
Contributor Author

Okay, let me explain:

I decided to remove the field, even though its public, and strictly speaking a breaking API change. However, it was a mistake to mark the field as public (back then, when it was added). It was only used internally as a configuration key for storing the serialized execution config (so it was something only used by flink).
It doesn't have any use for our users at all, so I think its absolutely safe to remove it. Its also not mentioned in the documentation.

I've added this exclude to the API check to ignore the breaking change:
b0acd97#diff-40616c4678c3fbfe07c0701505ce0567

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants