You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/05/18 19:56:26 UTC
[GitHub] [arrow] lidavidm opened a new pull request, #13191: ARROW-16592: [C++][Python][FlightRPC] Finish after failed writes
lidavidm opened a new pull request, #13191:
URL: https://github.com/apache/arrow/pull/13191
Currently a failed write (due to the server sending an error,
disconnecting, etc.) will raise an uninformative error on the
client. Prior to the refactoring done in Arrow 8.0.0, this was
silently swallowed (so clients would not get any indication until
they finished writing). In 8.0.0 instead the error got propagated
but this led to confusing, uninformative errors. Instead, tag this
specific error so that the client implementation knows to finish
the call and get the actual server error.
(gRPC doesn't give us the actual error until we explicitly finish
the call, so we can't get the actual error directly.)
--
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 a diff in pull request #13191: ARROW-16592: [C++][Python][FlightRPC] Finish after failed writes
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13191:
URL: https://github.com/apache/arrow/pull/13191#discussion_r878059568
##########
cpp/src/arrow/flight/transport/grpc/grpc_client.cc:
##########
@@ -350,16 +347,7 @@ class WritableDataStream : public FinishableDataStream<Stream, ReadPayload> {
Status DoFinish() override {
// This may be used concurrently by reader/writer side of a
// stream, so it needs to be protected.
- std::lock_guard<std::mutex> guard(finish_mutex_);
-
- // Now that we're shared between a reader and writer, we need to
- // protect ourselves from being called while there's an
- // outstanding read.
- std::unique_lock<std::mutex> read_guard(read_mutex_, std::try_to_lock);
- if (!read_guard.owns_lock()) {
- return MakeFlightError(FlightStatusCode::Internal,
- "Cannot close stream with pending read operation.");
- }
+ std::lock_guard<std::mutex> guard(read_mutex_);
Review Comment:
Basically, we have three choices of behavior when gRPC returns `false` on a write (write failed, but no error info):
1. Swallow the error and continue on until the user eventually calls Finish to get the error. (Arrow <= 7)
2. Raise a generic error to interrupt the writer, the user must catch this and still call Finish (Arrow 8)
3. Call Finish for the user and raise the actual error, _presumably_ this will complete instantly as the reason why `false` was returned means that there is already an error (this PR)
`read_mutex` is for any read operation which includes `Finish` (this is implicitly a read)
--
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 #13191: ARROW-16592: [C++][Python][FlightRPC] Finish after failed writes
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13191:
URL: https://github.com/apache/arrow/pull/13191#issuecomment-1130476915
https://issues.apache.org/jira/browse/ARROW-16592
--
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] cyb70289 commented on a diff in pull request #13191: ARROW-16592: [C++][Python][FlightRPC] Finish after failed writes
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on code in PR #13191:
URL: https://github.com/apache/arrow/pull/13191#discussion_r877704387
##########
cpp/src/arrow/flight/transport/grpc/grpc_client.cc:
##########
@@ -350,16 +347,7 @@ class WritableDataStream : public FinishableDataStream<Stream, ReadPayload> {
Status DoFinish() override {
// This may be used concurrently by reader/writer side of a
// stream, so it needs to be protected.
- std::lock_guard<std::mutex> guard(finish_mutex_);
-
- // Now that we're shared between a reader and writer, we need to
- // protect ourselves from being called while there's an
- // outstanding read.
- std::unique_lock<std::mutex> read_guard(read_mutex_, std::try_to_lock);
- if (!read_guard.owns_lock()) {
- return MakeFlightError(FlightStatusCode::Internal,
- "Cannot close stream with pending read operation.");
- }
+ std::lock_guard<std::mutex> guard(read_mutex_);
Review Comment:
Compared with original code, looks we will wait for reading finishes instead of fail immediately. Is it a problem?
Nit: IIUC `read_mutex` is to guard concurrent `read` to `finish`. If that's the case, the name may be a bit confusing, it sounds like to guard concurrent `read` to `read`. Is `finish_mutex` a better name?
--
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] cyb70289 merged pull request #13191: ARROW-16592: [C++][Python][FlightRPC] Finish after failed writes
Posted by GitBox <gi...@apache.org>.
cyb70289 merged PR #13191:
URL: https://github.com/apache/arrow/pull/13191
--
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