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.

Sign up for GitHub

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

Jump to bottom

Retry on Pinot Controller/Broker Rest calls and check data readiness for BasePinotIntegrationConnectorSmokeTest #14304

Closed
wants to merge 1 commit into from
Closed

Retry on Pinot Controller/Broker Rest calls and check data readiness for BasePinotIntegrationConnectorSmokeTest #14304

wants to merge 1 commit into from

Conversation

xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Sep 26, 2022 •
edited
Loading

Fix #14277.

  • Retry on Pinot Controller/Broker Rest calls
  • Check data readiness for BasePinotIntegrationConnectorSmokeTest

@cla-bot cla-bot bot added the cla-signed label Sep 26, 2022
@xiangfu0 xiangfu0 mentioned this pull request Sep 26, 2022
Closed
@xiangfu0 xiangfu0 force-pushed the flaky-trino-pinot-test branch from ff80030 to 4841cc4 Compare September 27, 2022 00:25
@xiangfu0
Copy link
Contributor Author

Seems the stress test passed with 50 tests.
https://github.com/trinodb/trino/actions/runs/3132039012/jobs/5083980106

If you feel ok, I can remove the commit 2?

also cc: @martint @findepi

@xiangfu0 xiangfu0 force-pushed the flaky-trino-pinot-test branch from 4841cc4 to cf92424 Compare September 27, 2022 01:04
@xiangfu0 xiangfu0 force-pushed the flaky-trino-pinot-test branch 2 times, most recently from 2767bfd to 4959061 Compare September 27, 2022 08:08
@xiangfu0 xiangfu0 marked this pull request as draft September 27, 2022 08:08
@findepi
Copy link
Member

findepi commented Sep 27, 2022

thanks for the PR

Query pinot to ensure both tables are ready in the test init.
#14277

If this is supposed to fix the issue, can you please put "fixes #14277" in the PR description?

@xiangfu0 xiangfu0 force-pushed the flaky-trino-pinot-test branch from b0825b3 to f91c25e Compare September 30, 2022 22:58
@xiangfu0 xiangfu0 changed the title Adding data ready check for BasePinotIntegrationConnectorSmokeTest Fix #14277, Retry on Pinot Controller/Broker Rest calls and check data readiness for BasePinotIntegrationConnectorSmokeTest Sep 30, 2022
@xiangfu0 xiangfu0 force-pushed the flaky-trino-pinot-test branch from f91c25e to e3985c7 Compare September 30, 2022 23:06
@xiangfu0
Copy link
Contributor Author

updated and try out more tests

@xiangfu0 xiangfu0 force-pushed the flaky-trino-pinot-test branch 7 times, most recently from 465ec6d to 956da44 Compare October 4, 2022 06:17
@xiangfu0 xiangfu0 closed this Oct 6, 2022
@xiangfu0 xiangfu0 reopened this Oct 6, 2022
@xiangfu0 xiangfu0 force-pushed the flaky-trino-pinot-test branch 2 times, most recently from 0fea595 to 4968214 Compare October 7, 2022 08:01
@xiangfu0 xiangfu0 closed this Oct 15, 2022
@xiangfu0 xiangfu0 reopened this Oct 15, 2022
@xiangfu0
Copy link
Contributor Author

Got 100 passes: https://github.com/trinodb/trino/actions/runs/3257550165/jobs/5348890727

@xiangfu0 xiangfu0 closed this Oct 16, 2022
@xiangfu0 xiangfu0 reopened this Oct 16, 2022
@xiangfu0
Copy link
Contributor Author

Another 100 passes: https://github.com/trinodb/trino/actions/runs/3258870972/jobs/5351289222

@xiangfu0 xiangfu0 force-pushed the flaky-trino-pinot-test branch from 8d19d74 to a21df5a Compare October 16, 2022 10:15
@xiangfu0
Copy link
Contributor Author

Another 100 pass:https://github.com/trinodb/trino/actions/runs/3259111250/jobs/5351713063

@xiangfu0
Copy link
Contributor Author

I feel this is the fix for #14277
Please take a look @ebyhr

@xiangfu0 xiangfu0 marked this pull request as ready for review October 17, 2022 07:09
@xiangfu0
Copy link
Contributor Author

I feel this is the fix for #14277
Please take a look @ebyhr
Got about 400 continuous passes.
Meanwhile the current test still has some error: #14324

@xiangfu0 xiangfu0 requested a review from ebyhr October 17, 2022 07:10
@xiangfu0 xiangfu0 force-pushed the flaky-trino-pinot-test branch 2 times, most recently from 80b62ed to 81fb9c7 Compare October 17, 2022 20:50
@xiangfu0 xiangfu0 force-pushed the flaky-trino-pinot-test branch 2 times, most recently from dc19376 to d29ed40 Compare October 18, 2022 05:26
ebyhr
ebyhr reviewed Oct 27, 2022
View reviewed changes
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java
return doWithRetries(retries, caller, DEFAULT_RETRY_INTERVAL);
}

public static <T> T doWithRetries(int retries, Function<Integer, T> caller, int retryInterval)
Copy link
Member

Choose a reason for hiding this comment

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

Why not using Failsafe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not familiar with this lib. Feel free to modify this PR or point me to the sample usage.

Copy link
Member

Choose a reason for hiding this comment

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

Here's an example usage: https://github.com/trinodb/trino/blob/master/plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/ReadSessionCreator.java#L111

@ebyhr ebyhr changed the title Fix #14277, Retry on Pinot Controller/Broker Rest calls and check data readiness for BasePinotIntegrationConnectorSmokeTest Retry on Pinot Controller/Broker Rest calls and check data readiness for BasePinotIntegrationConnectorSmokeTest Oct 27, 2022
@xiangfu0 xiangfu0 mentioned this pull request Oct 27, 2022
Open
@xiangfu0
Copy link
Contributor Author

#14239

@findepi findepi requested review from wendigo and hashhar and removed request for findepi October 27, 2022 12:50
@mosabua
Copy link
Member

mosabua commented Jan 12, 2024

👋 @xiangfu0 - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 4, 2024

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Sep 4, 2024
@mosabua
Copy link
Member

mosabua commented Sep 10, 2024

the linked issue is fixed. I am closing this PR. Feel free to reopen if anything is still missing and you want to proceed with rebasing and further work.

@mosabua mosabua closed this Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ebyhr ebyhr ebyhr left review comments

@martint martint Awaiting requested review from martint

@elonazoulay elonazoulay Awaiting requested review from elonazoulay

@wendigo wendigo Awaiting requested review from wendigo

@hashhar hashhar Awaiting requested review from hashhar

Assignees
No one assigned
Labels
cla-signed stale
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Flaky TestSecuredPinotIntegrationConnectorSmokeTest
5 participants
@xiangfu0 @findepi @mosabua @elonazoulay @ebyhr