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-30257] fix SqlClientITCase#testMatchRecognize #21696

Merged
merged 1 commit into from Feb 15, 2023

Conversation

WencongLiu
Copy link
Contributor

@WencongLiu WencongLiu commented Jan 17, 2023

What is the purpose of the change

fix SqlClientITCase#testMatchRecognize

Brief change log

  • make verifyNumberOfResultRecords wait until getting the expected number of records

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: (yes)
  • 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 documented)

@flinkbot
Copy link
Collaborator

flinkbot commented Jan 17, 2023

CI report:

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

int numberOfResultRecords = UpsertTestFileUtil.getNumberOfRecords(tempOutputFile);
int numberOfResultRecords;
while (true) {
Thread.sleep(5000); // prevent NotFoundException: Status 404
Copy link
Contributor

Choose a reason for hiding this comment

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

@zentol Any thoughts if/how we could get rid of the wait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @zentol

Copy link
Contributor

Choose a reason for hiding this comment

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

At the end of the way you will need to poll something, be it for the output file or the status of the job.

But the sleep could be way lower; 50ms would be perfectly fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@WencongLiu Could you lower the sleep?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MartijnVisser
Copy link
Contributor

@WencongLiu You'll need to rebase your PR with the latest changes from master to get the CI to pass.

@WencongLiu
Copy link
Contributor Author

@flinkbot run azure

@WencongLiu
Copy link
Contributor Author

WencongLiu commented Feb 15, 2023

@zentol @MartijnVisser Done.

Copy link
Contributor

@MartijnVisser MartijnVisser left a comment

Choose a reason for hiding this comment

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

@WencongLiu Thanks! Will you also create backports to release-1.17 and potentially release-1.16 ?

@MartijnVisser MartijnVisser merged commit 4595762 into apache:master Feb 15, 2023
@WencongLiu
Copy link
Contributor Author

@WencongLiu Thanks! Will you also create backports to release-1.17 and potentially release-1.16 ?

Do you mean to create the same pull request for release-1.17 and release-1.16?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants