Skip to content

[FLINK-37720][table-planner] Enable sink reuse table optimizer by default and apply plan changes to existing test cases #26503

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

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

Conversation

xiangyuf
Copy link
Contributor

@xiangyuf xiangyuf commented Apr 24, 2025

What is the purpose of the change

Enable sink reuse table optimizer by default and apply plan changes to existing test cases

Brief change log

Enable sink reuse table optimizer by default and apply plan changes to existing test cases

Verifying this change

This change is already covered by existing tests

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

@flinkbot
Copy link
Collaborator

flinkbot commented Apr 24, 2025

CI report:

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

@davidradl
Copy link
Contributor

@xiangyuf I am wondering what the migration implications are to change this to be default. In general it is not good practise to change defaults, but I see the flip has not being fully implemented so maybe this is not a concern. Please can you let me know your thinking; maybe this change in behaviour is always desirable and would not be seen as a break for existing users?

@xiangyuf
Copy link
Contributor Author

@xiangyuf I am wondering what the migration implications are to change this to be default. In general it is not good practise to change defaults, but I see the flip has not being fully implemented so maybe this is not a concern. Please can you let me know your thinking; maybe this change in behaviour is always desirable and would not be seen as a break for existing users?

Hi David, as discussed in this thread https://lists.apache.org/thread/r1wo9sf3d1725fhwzrttvv56k4rc782m, the sink reuse is enabled by default in the first place for following reason:
"taking sink reuse as a long awaited feature with significant benefits for users, we choose to enable this in the first place. Similar features like table.optimizer.reuse-sub-plan-enabled and table.optimizer.reuse-source-enabled are also enabled by default. From this point of view, sink reuse should be the same."

Also, this PR is a draft PR, I need to change several existing IT cases to pass the CI.

@xiangyuf xiangyuf force-pushed the FLINK-37720 branch 3 times, most recently from fdf7bf6 to aae9bd9 Compare April 25, 2025 07:04
@xiangyuf xiangyuf changed the title [FLINK-37720][table-planner] Set the default value of table.optimizer.reuse-sink-enabled as true [FLINK-37720][table-planner] Enable sink reuse table optimizer by default and apply plan changes to existing test cases Apr 25, 2025
@xiangyuf xiangyuf force-pushed the FLINK-37720 branch 3 times, most recently from 4c18e50 to 4e6af9d Compare April 25, 2025 07:24
@xiangyuf
Copy link
Contributor Author

@davidradl Hi, this pr is ready to merge now. Welcome any reviews.

@xiangyuf xiangyuf force-pushed the FLINK-37720 branch 3 times, most recently from 1bc99e1 to b92435e Compare April 25, 2025 17:57
…ault and apply plan changes to existing test cases
@xiangyuf
Copy link
Contributor Author

@xuyangzhong @reswqa Kindly remind for review.

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