You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lidavidm (via GitHub)" <gi...@apache.org> on 2023/03/06 20:54:06 UTC
[GitHub] [arrow] lidavidm opened a new pull request, #34473: GH-34772: [Go][FlightRPC] Drain result of DoAction in Flight SQL client
lidavidm opened a new pull request, #34473:
URL: https://github.com/apache/arrow/pull/34473
### Rationale for this change
Unless we drain the result of DoAction, we can't be sure that the RPC actually completed.
### What changes are included in this PR?
Always drain the result of DoAction.
### Are these changes tested?
A new test was added and existing tests were modified.
### Are there any user-facing changes?
This fixes potential nondeterministic behavior that users may have previously observed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34473: GH-34772: [Go][FlightRPC] Drain result of DoAction in Flight SQL client
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34473:
URL: https://github.com/apache/arrow/pull/34473#issuecomment-1456977059
:x: GitHub issue #34772 could not be retrieved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zeroshade commented on a diff in pull request #34473: GH-34472: [Go][FlightRPC] Drain result of DoAction in Flight SQL client
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #34473:
URL: https://github.com/apache/arrow/pull/34473#discussion_r1127032024
##########
go/arrow/flight/flightsql/client.go:
##########
@@ -500,6 +522,11 @@ func (c *Client) CancelQuery(ctx context.Context, info *flight.FlightInfo, opts
return
}
defer stream.CloseSend()
+ defer func() {
+ // XXX: assuming server will not return a result and then an error
+ // (or else we need to also try to clean up stmt)
+ err = readUntilEOF(stream)
+ }()
Review Comment:
rather than defer this, why not just do it manually after the next if statement?
##########
go/arrow/flight/flightsql/client.go:
##########
@@ -396,6 +408,11 @@ func (c *Client) Prepare(ctx context.Context, query string, opts ...grpc.CallOpt
return
}
defer stream.CloseSend()
+ defer func() {
+ // XXX: assuming server will not return a result and then an error
+ // (or else we need to also try to clean up stmt)
+ err = readUntilEOF(stream)
+ }()
Review Comment:
would it make more sense to put this into `parsePreparedStatementResponse` and get it for free in all the spots that just delegate to it?
##########
go/arrow/flight/flightsql/client.go:
##########
@@ -806,10 +845,7 @@ func (tx *Txn) Rollback(ctx context.Context, opts ...grpc.CallOption) error {
}
tx.txn = nil
- if _, err = stream.Recv(); err == io.EOF {
- err = nil
- }
-
+ err = readUntilEOF(stream)
return err
Review Comment:
same as above
##########
go/arrow/flight/flightsql/client.go:
##########
@@ -775,9 +816,7 @@ func (tx *Txn) Commit(ctx context.Context, opts ...grpc.CallOption) error {
}
tx.txn = nil
- if _, err = stream.Recv(); err == io.EOF {
- err = nil
- }
+ err = readUntilEOF(stream)
return err
Review Comment:
`return readUntilEOF(stream)` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34473: GH-34472: [Go][FlightRPC] Drain result of DoAction in Flight SQL client
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34473:
URL: https://github.com/apache/arrow/pull/34473#issuecomment-1456979434
* Closes: #34472
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #34473: GH-34472: [Go][FlightRPC] Drain result of DoAction in Flight SQL client
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34473:
URL: https://github.com/apache/arrow/pull/34473#issuecomment-1468858267
Benchmark runs are scheduled for baseline = f2cc0b41fe9fb1d8d2bdb1d2abf676278e273f55 and contender = 6fdf1e520a761ee833f97241b3dd6687d23ef5ef. 6fdf1e520a761ee833f97241b3dd6687d23ef5ef is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/6560a669628a4515a77307a2e3778a91...03bfe40d48064748b99bcba87b2851d6/)
[Failed :arrow_down:1.5% :arrow_up:0.18%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/8979664d2cdb4dfd82bf132bd82d36e1...6e031295fe6d4c73a747df5740a888e3/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/b1db1e82dd824f93bcb87c86d544781d...07d4e47e303543509e6a1df22778a88f/)
[Finished :arrow_down:0.19% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/30eab4d7f6914d0da944bf3d5b56d6cc...679fc20b962f449ab4ff8cd4734c11fe/)
Buildkite builds:
[Finished] [`6fdf1e52` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2489)
[Failed] [`6fdf1e52` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2519)
[Finished] [`6fdf1e52` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2487)
[Finished] [`6fdf1e52` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2510)
[Finished] [`f2cc0b41` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2488)
[Finished] [`f2cc0b41` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2518)
[Finished] [`f2cc0b41` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2486)
[Finished] [`f2cc0b41` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2509)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #34473: GH-34472: [Go][FlightRPC] Drain result of DoAction in Flight SQL client
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34473:
URL: https://github.com/apache/arrow/pull/34473#issuecomment-1457035240
Cleaned things up:
- CloseSend() needs to be called before Recv() always
- Mocks were made more reliable via Once()
- Addressed comments
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #34473: GH-34472: [Go][FlightRPC] Drain result of DoAction in Flight SQL client
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34473:
URL: https://github.com/apache/arrow/pull/34473#issuecomment-1458640969
Whoops, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm merged pull request #34473: GH-34472: [Go][FlightRPC] Drain result of DoAction in Flight SQL client
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #34473:
URL: https://github.com/apache/arrow/pull/34473
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #34473: GH-34472: [Go][FlightRPC] Drain result of DoAction in Flight SQL client
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #34473:
URL: https://github.com/apache/arrow/pull/34473#issuecomment-1458792141
I'll merge this now, CI failures are from #34488
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org