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

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

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

wants to merge 1 commit into from
+151 −73

Conversation

xiangfu0
Copy link
Contributor

@xiangfu0 xiangfu0 commented Sep 26, 2022 •
edited

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
Open
@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 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 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
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 force-pushed the flaky-trino-pinot-test branch 3 times, most recently from 9c82fb9 to 8d19d74 Compare October 15, 2022 09:26
@xiangfu0
Copy link
Contributor Author

Around of 100 passes: https://github.com/trinodb/trino/actions/runs/3255177903/jobs/5344234451

@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
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.

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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