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-37511][rest] Use Jackson serialization in JobPlanInfo.Plan #26320

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Efrat19
Copy link

@Efrat19 Efrat19 commented Mar 19, 2025

What is the purpose of the change

The pr follows refactors done over time to transition rest object classes to use modern auto-serialization instead of implementing their own serializations.
Doing the same for org.apache.flink.runtime.rest.messages.JobPlanInfo.Plan improves error handling and makes the restAPI more informative about expected response.

Brief change log

  • org.apache.flink.runtime.rest.messages.JobPlanInfo.Plan type refactored to object instead of a string
  • References updated as well

Verifying this change

This change is already covered by existing tests, such as (please describe tests).

  • org.apache.flink.runtime.jobgraph.jsonplan.JsonGeneratorTest#testGeneratorWithoutAnyAttachements
  • org.apache.flink.runtime.rest.messages.JobPlanInfoTest#testJsonMarshalling

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: Not sure which serializers we are talking about here
  • 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

Open questions / follow ups

To be able to completely get rid of org.apache.flink.runtime.rest.messages.JobPlanInfo.RawJson#RawJson I would need to refactor StreamGraphs as well - Should probably separate it to a follow up PR to reduce blast radius? cc @zentol

@flinkbot
Copy link
Collaborator

flinkbot commented Mar 19, 2025

CI report:

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

@Efrat19 Efrat19 changed the title [FLINK-37511] Use Jackson serialization in JobPlanInfo.Plan [FLINK-37511][rest] Use Jackson serialization in JobPlanInfo.Plan Mar 19, 2025
@davidradl
Copy link
Contributor

  • this appears to be an issue with the open api and rest docs - that are not generating correctly.
  • the fix seems to be to change the way we do job serialization
  • on the face of it this seems a large change to a core part of Flink, that could introduce other considerations for example how are existing job serializations effected.

Have I understood this correctly?

@Efrat19 Efrat19 force-pushed the FLINK-37511 branch 5 times, most recently from 66a5865 to 2244538 Compare March 24, 2025 16:37
@Efrat19
Copy link
Author

Efrat19 commented Mar 25, 2025

@flinkbot run azure

@Efrat19 Efrat19 marked this pull request as ready for review March 25, 2025 12:25
fixes

generate-rest-snapshot

generate-rest-snapshot

generate-rest-snapshot
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