flink icon indicating copy to clipboard operation
flink copied to clipboard

Published 20 hours ago • verified publisher icon apache

[FLINK-28324][flink-sql-client][JUnit5 Migration] Module: flink-sql-client

Open zhoulii opened this issue 2 years ago • 2 comments

What is the purpose of the change

[JUnit5 Migration] Module: flink-sql-client

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): (yes / no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
  • The serializers: (yes / no / don't know)
  • The runtime per-record code paths (performance sensitive): (yes / no / don't know)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (yes / no / don't know)
  • The S3 file system connector: (yes / no / don't know)

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

zhoulii avatar Jul 05 '22 10:07 zhoulii

CI report:

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

flinkbot avatar Jul 05 '22 11:07 flinkbot

Thanks for your contribution I left some comments I fear you need to rebase it since there are conflicts...

snuyanzin avatar Sep 12 '22 09:09 snuyanzin

@zhoulii are you able to work on this considering @snuyanzin 's review comments?

XComp avatar Oct 07 '22 07:10 XComp

Thanks for opening this PR! @zhoulii Now the Flink master is ready to merge changes for Flink 1.17 and we can continue with this migration work. Would you rebase the change to the lastest master?

zhuzhurk avatar Oct 18 '22 02:10 zhuzhurk

Thanks for reviewing, I will rebase the change and address the comments.

zhoulii avatar Oct 18 '22 02:10 zhoulii

Thanks for your contribution @zhoulii. Just a minor comment about the timeouts: Keep in mind that we actually want to remove timeouts from JUnit tests (see coding guidelines) to benefit from the thread dump printout generated by our CI tools. So it might be better to remove them instead of migrating them to JUnit5

XComp avatar Oct 18 '22 05:10 XComp

@flinkbot run azure

zhoulii avatar Oct 19 '22 05:10 zhoulii

@flinkbot run azure

zhoulii avatar Oct 19 '22 08:10 zhoulii

Hi @zhuzhurk, I have rebased the change to the latest master, the ci passed now.

zhoulii avatar Oct 20 '22 02:10 zhoulii

Thanks for working on this! @zhoulii And thanks for the thorough review! @snuyanzin Merging.

zhuzhurk avatar Oct 20 '22 05:10 zhuzhurk