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-29787][ci] fix ci METHOD_NEW_DEFAULT issue #21184

Merged
merged 2 commits into from
Oct 28, 2022

Conversation

liyubin117
Copy link
Contributor

What is the purpose of the change

org.apache.flink.api.connector.source.SourceReader declared a new default function pauseOrResumeSplits(), japicmp plugin failed during ci running, we should configure the plugin to make it compatible.

Brief change log

  • configure overrideCompatibilityChangeParameter

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, Kubernetes/Yarn/Mesos, ZooKeeper: no
  • The S3 file system connector: no

Documentation

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

@flinkbot
Copy link
Collaborator

flinkbot commented Oct 28, 2022

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

Copy link
Contributor

@XComp XComp left a comment

Choose a reason for hiding this comment

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

I'm not sure whether that's the desired solution to fix the CI failures on master: I'm not 100% sure but I understand tools/releasing/update_japicmp_configuration.sh:40 in a way that we should only add the @PublicEvolving constraints on the release branch but not in master.

  • enable stronger compatibility constraints for X.Y-SNAPSHOT to ensure compatibility for PublicEvolving

The documentation is a bit confusing here in my opinion (which we might want to fix making the docs more explicit by mentioning whether it should be applied to the release branch and/or master). Essentially, we would want to revert the inclusion of the @PublicEvolving rule that was introduced with 82567cc9 on master. @HuangXingBo am I right? 🤔

@XComp
Copy link
Contributor

XComp commented Oct 28, 2022

Thanks, @liyubin117 . Could you squash the commits into two? I would merge the PR then right away to unblock master. We can still revert this in case @HuangXingBo or somebody else objects this change.

@liyubin117
Copy link
Contributor Author

@XComp Squash done. Thanks for your excellent idea :)

@XComp XComp merged commit cb44293 into apache:master Oct 28, 2022
@XComp
Copy link
Contributor

XComp commented Oct 28, 2022

The commits got a bit mixed up. I decided to squash everything into one because the documentation change should be reverted as well if the change itself was wrong. I merged the PR without waiting for CI to unblock master.

Thanks for looking into it @liyubin117 :-)

@WencongLiu
Copy link
Contributor

WencongLiu commented Aug 31, 2023

Hello @XComp @liyubin117 , currently I meet a same problem when I'm trying to add a new default method to an interface with @ Public annotation. Should I just need to add an @PublicEvolving annotation to the new added default method to avoid this?

BTW, according to the API compatibility guarantees in flink website, we shouldn't check binary incompatible between minor versions flink/pom.xml. We need to open a issue to fix it.

@XComp
Copy link
Contributor

XComp commented Aug 31, 2023

The default method covers the old implementation? That shouldn't be degraded to @PublicEvolving. 🤔

BTW, according to the API compatibility guarantees in flink website, we shouldn't check binary incompatible between minor versions flink/pom.xml. We need to open a issue to fix it.

I think you're right. I created FLINK-33009 to cover this. Let's keep the discussion there.

@WencongLiu
Copy link
Contributor

WencongLiu commented Aug 31, 2023

Thanks @XComp . 😄

The default method covers the old implementation? That shouldn't be degraded to @PublicEvolving.

Apologies for any confusion caused. I just want to add a new default method to an existed @ Public interface that already have several old methods. I think this behavior shouldn't break source compatibility. I have reported this to japicmp community and the community owner replied that they will fix this in the next release. The LINK is here.

I think you're right. I created FLINK-33009 to cover this. Let's keep the discussion there.

I will discuss in FLINK-33009.

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.

4 participants