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-30454 [runtime] Fixing visibility issue for SizeGauge.SizeSupplier #21531

Closed
wants to merge 1 commit into from

Conversation

gunnarmorling
Copy link
Contributor

@gunnarmorling gunnarmorling commented Dec 19, 2022

https://issues.apache.org/jira/browse/FLINK-30454

What is the purpose of the change

Making sure the runtime module can be compiled with ecj. I encountered two locations which seem inconsistent with the JLS but were still accepted by javac.

Brief change log

Reworked two code places to make it compile with ecj. Note the change around SizeGauge.SizeSupplier is source-compatible but not binary-compatible, based on the assumption that this code is not invoked in external client code. If that actually is the case, I'd rework the change to simply change the visibility of SizeGauge to public.

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

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, 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: not applicable

Also changing visibility JsonJobResultEntry.FIELD_NAME_VERSION as per JLS§6.6.1 requirements.
@flinkbot
Copy link
Collaborator

flinkbot commented Dec 19, 2022

CI report:

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

@gunnarmorling
Copy link
Contributor Author

// CC @rmetzger

@@ -207,7 +207,7 @@ public Set<JobResult> getDirtyResultsInternal() throws IOException {
@VisibleForTesting
static class JsonJobResultEntry extends JobResultEntry {
private static final String FIELD_NAME_RESULT = "result";
private static final String FIELD_NAME_VERSION = "version";
static final String FIELD_NAME_VERSION = "version";
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The field couldn't be referenced in the annotation otherwise when compiling with ecj. I suppose it's another case where javac is a bit too lenient.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I see.

@rmetzger
Copy link
Contributor

I'm manually merging this PR, adjusting the commit message to adhere to the Flink pattern

@rmetzger rmetzger closed this in f82be84 Dec 22, 2022
chucheng92 pushed a commit to chucheng92/flink that referenced this pull request Feb 3, 2023
Also changing visibility JsonJobResultEntry.FIELD_NAME_VERSION as per JLS§6.6.1 requirements.

This closes apache#21531
akkinenivijay pushed a commit to krisnaru/flink that referenced this pull request Feb 11, 2023
Also changing visibility JsonJobResultEntry.FIELD_NAME_VERSION as per JLS§6.6.1 requirements.

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