Fix Flaky Test in Clickhouse CI#26080
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjust the ClickHouse test setup to bind to an ephemeral HTTP port instead of a fixed port to prevent CI test flakes due to port conflicts. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Add a brief comment explaining that port "0" is used to request an ephemeral port and avoid conflicts in CI.
- Verify that the ClickHouse query runner properly reads and propagates the dynamically assigned port rather than assuming a fixed value.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add a brief comment explaining that port "0" is used to request an ephemeral port and avoid conflicts in CI.
- Verify that the ClickHouse query runner properly reads and propagates the dynamically assigned port rather than assuming a fixed value.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
imjalpreet
left a comment
There was a problem hiding this comment.
Thank you, @nishithakbhaskaran. I just faced the issue and saw you already fixed it.
| this.clickhouseServer = new TestingClickHouseServer(); | ||
| return createClickHouseQueryRunner(clickhouseServer, | ||
| ImmutableMap.of("http-server.http.port", "8080"), | ||
| ImmutableMap.of("http-server.http.port", "0"), |
There was a problem hiding this comment.
I think this is handled in TestingPrestoServer as well. If we just don't add this property, it will default to 0. Can we try that too?
There was a problem hiding this comment.
Sure, let me try out.
e622291 to
a074d64
Compare
tdcmeehan
left a comment
There was a problem hiding this comment.
Thank you @nishithakbhaskaran
imjalpreet
left a comment
There was a problem hiding this comment.
Thanks, @nishithakbhaskaran. Just a small suggestion.
| @@ -61,7 +61,7 @@ protected QueryRunner createQueryRunner() | |||
| { | |||
| this.clickhouseServer = new TestingClickHouseServer(); | |||
| return createClickHouseQueryRunner(clickhouseServer, | |||
There was a problem hiding this comment.
It looks like it will work. We can refactor to use the other constructor
There was a problem hiding this comment.
Updated.PTAL
926a6f1 to
9178621
Compare
9178621 to
d5e037b
Compare
Description
Fix flaky test in Clickhouse CI
Motivation and Context
Resolves #26079
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.