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-31673][jdbc-driver] Add e2e test for flink jdbc driver #22583

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

FangYongs
Copy link
Contributor

What is the purpose of the change

Add e2e test for flink jdbc driver

Brief change log

  • Add FlinkDriverE2eTest

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

Documentation

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

@flinkbot
Copy link
Collaborator

flinkbot commented May 15, 2023

CI report:

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

@FangYongs
Copy link
Contributor Author

@flinkbot run azure

@FangYongs
Copy link
Contributor Author

Hi @libenchao Please have a look at this PR when you are free, thanks

@libenchao libenchao self-assigned this May 29, 2023
@libenchao
Copy link
Member

@FangYongs By saying "e2e test", I mean the tests in flink-end-to-end-tests module. What do you think about adding it there?

@FangYongs
Copy link
Contributor Author

Thanks @libenchao Sounds good to me, I'll move it there

@FangYongs FangYongs force-pushed the FLINK_31673_e2e_test branch 2 times, most recently from 1ccbf49 to da2db9f Compare June 6, 2023 05:36
@FangYongs
Copy link
Contributor Author

Hi @libenchao I have update this PR for e2e test, please help to review when you are free, thanks

Copy link
Member

@libenchao libenchao left a comment

Choose a reason for hiding this comment

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

@FangYongs Thanks for your updating, but in 'flink-end-to-end-tests', we usually run these tests via scripts, such as flink-end-to-end-tests/test-scripts/test_batch_sql.sh, in that way, we'll gain the benefits such as:

  • The test will be run as a normal program instead of running in Junit environment, with real flink cluster (local cluster instead of mini cluster)
  • These tests will be run in the special stage, not in the maven test stage, which is more flexible

@FangYongs
Copy link
Contributor Author

Hi @libenchao I have rebased master and updated this PR, please have a look again when you are free, thanks

Copy link
Member

@libenchao libenchao left a comment

Choose a reason for hiding this comment

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

Looks good to me, I only have one minor comment.

@@ -168,7 +168,6 @@ public ShutdownThread(SqlGateway gateway) {
@Override
public void run() {
// Shutdown the gateway
System.out.println("\nShutting down the Flink SqlGateway...");
Copy link
Member

Choose a reason for hiding this comment

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

Is this change expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@libenchao Yes, it will cause e2e test failed because the e2e test framework will check the .out log file and confirm that it is empty. I have confirmed this with @fsk119 that the System.out.println here is meaningless, we can remove them directly here.

Copy link
Member

Choose a reason for hiding this comment

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

sounds good to me.

@libenchao libenchao merged commit 089e9ed into apache:master Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants