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

GH-35328: [Go][FlightSQL] Fix flaky test for FlightSql driver #38044

Merged
merged 1 commit into from
Oct 5, 2023

Conversation

zeroshade
Copy link
Member

@zeroshade zeroshade commented Oct 5, 2023

Rationale for this change

Fixing a flaky test that is very difficult to reproduce.

What changes are included in this PR?

Adding an explicit call to drain the remaining batches from the parameter batch reader in the DoPutPreparedStatementQuery of the MockServer in the tests. This ensures that the gRPC connection doesn't close on us in between the client writing the schema message and writing the record batch message producing an io.EOF error. This is an extremely rare occurence based on goroutine scheduling due to the client and mockserver both running in the same process for the tests, in local testing i was only able to get it to happen 1 - 4 times per 5000 runs of the test.

Are there any user-facing changes?

No

@zeroshade zeroshade requested a review from lidavidm October 5, 2023 16:03
@github-actions
Copy link

github-actions bot commented Oct 5, 2023

⚠️ GitHub issue #35328 has been automatically assigned in GitHub to PR creator.

@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Oct 5, 2023
@zeroshade zeroshade merged commit 1cad7a7 into apache:main Oct 5, 2023
24 checks passed
@zeroshade zeroshade removed the awaiting merge Awaiting merge label Oct 5, 2023
@zeroshade zeroshade deleted the fix-flaky-test branch October 5, 2023 16:49
@conbench-apache-arrow
Copy link

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 1cad7a7.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 7 possible false positives for unstable benchmarks that are known to sometimes produce them.

JerAguilon pushed a commit to JerAguilon/arrow that referenced this pull request Oct 23, 2023
…pache#38044)

### Rationale for this change
Fixing a flaky test that is *very* difficult to reproduce.

### What changes are included in this PR?
Adding an explicit call to drain the remaining batches from the parameter batch reader in the `DoPutPreparedStatementQuery` of the `MockServer` in the tests. This ensures that the gRPC connection doesn't close on us in between the client writing the schema message and writing the record batch message producing an `io.EOF` error. This is an extremely rare occurence based on goroutine scheduling due to the client and mockserver both running in the same process for the tests, in local testing i was only able to get it to happen 1 - 4 times per 5000 runs of the test.

### Are there any user-facing changes?
No

* Closes: apache#35328

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
loicalleyne pushed a commit to loicalleyne/arrow that referenced this pull request Nov 13, 2023
…pache#38044)

### Rationale for this change
Fixing a flaky test that is *very* difficult to reproduce.

### What changes are included in this PR?
Adding an explicit call to drain the remaining batches from the parameter batch reader in the `DoPutPreparedStatementQuery` of the `MockServer` in the tests. This ensures that the gRPC connection doesn't close on us in between the client writing the schema message and writing the record batch message producing an `io.EOF` error. This is an extremely rare occurence based on goroutine scheduling due to the client and mockserver both running in the same process for the tests, in local testing i was only able to get it to happen 1 - 4 times per 5000 runs of the test.

### Are there any user-facing changes?
No

* Closes: apache#35328

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
dgreiss pushed a commit to dgreiss/arrow that referenced this pull request Feb 19, 2024
…pache#38044)

### Rationale for this change
Fixing a flaky test that is *very* difficult to reproduce.

### What changes are included in this PR?
Adding an explicit call to drain the remaining batches from the parameter batch reader in the `DoPutPreparedStatementQuery` of the `MockServer` in the tests. This ensures that the gRPC connection doesn't close on us in between the client writing the schema message and writing the record batch message producing an `io.EOF` error. This is an extremely rare occurence based on goroutine scheduling due to the client and mockserver both running in the same process for the tests, in local testing i was only able to get it to happen 1 - 4 times per 5000 runs of the test.

### Are there any user-facing changes?
No

* Closes: apache#35328

Authored-by: Matt Topol <zotthewizard@gmail.com>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Go] FlightSQL TestPreparedStatementNoSchema test is failing
2 participants