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/12/29 20:23:27 UTC
[GitHub] [arrow] lidavidm opened a new pull request, #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
lidavidm opened a new pull request, #15118:
URL: https://github.com/apache/arrow/pull/15118
Fixes #15069.
--
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 #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #15118:
URL: https://github.com/apache/arrow/pull/15118#issuecomment-1368543291
Thanks for catching this, Kou. Either we need to revert this PR or remove that test, since the two behaviors conflict with each other.
--
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 #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #15118:
URL: https://github.com/apache/arrow/pull/15118#issuecomment-1368991073
Created #15150
--
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] kou commented on pull request #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
kou commented on PR #15118:
URL: https://github.com/apache/arrow/pull/15118#issuecomment-1368377678
@lidavidm @wjones127 This might break `wheel-macos-big-sur-*`. For example: https://github.com/ursacomputing/crossbow/actions/runs/3815472986/jobs/6490471956#step:11:435
```text
=================================== FAILURES ===================================
_________________________ test_none_action_side_effect _________________________
def test_none_action_side_effect():
"""Ensure that actions are executed even when we don't consume iterator.
See https://issues.apache.org/jira/browse/ARROW-14255
"""
with ActionNoneFlightServer() as server, \
FlightClient(('localhost', server.port)) as client:
client.do_action(flight.Action("append", b""))
r = client.do_action(flight.Action("get_value", b""))
> assert json.loads(next(r).body.to_pybytes()) == [True]
E assert [] == [True]
E Right contains one more item: True
E Full diff:
E - [True]
E + []
test-arm64-env/lib/python3.10/site-packages/pyarrow/tests/test_flight.py:2132: AssertionError
----------------------------- Captured stderr call -----------------------------
/Users/voltrondata/github-actions-runner/_work/crossbow/crossbow/arrow/cpp/src/arrow/status.cc:137: DoAction result was not fully consumed: Cancelled: Flight cancelled call, with message: CANCELLED. Detail: Cancelled
```
--
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 #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15118:
URL: https://github.com/apache/arrow/pull/15118#issuecomment-1367569054
* Closes: #15069
--
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 #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #15118:
URL: https://github.com/apache/arrow/pull/15118#discussion_r1059531310
##########
cpp/src/arrow/flight/flight_test.cc:
##########
@@ -1286,11 +1289,12 @@ TEST_F(TestTls, OverrideHostname) {
ASSERT_OK_AND_ASSIGN(auto client, FlightClient::Connect(location_, client_options));
FlightCallOptions options;
- options.timeout = TimeoutDuration{5.0};
+ // options.timeout = TimeoutDuration{5.0};
Review Comment:
Thanks, forgot to take out the debugging stuff.
--
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 #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15118:
URL: https://github.com/apache/arrow/pull/15118#issuecomment-1368359057
Revision: 1e759365f16047b16fea87a3800128c930c968f0
Submitted crossbow builds: [ursacomputing/crossbow @ actions-d461a7b52e](https://github.com/ursacomputing/crossbow/branches/all?query=actions-d461a7b52e)
|Task|Status|
|----|------|
|wheel-macos-big-sur-cp310-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815472986/jobs/6490471956)|
|wheel-macos-big-sur-cp311-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815473376/jobs/6490472658)|
|wheel-macos-big-sur-cp38-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815472386/jobs/6490470982)|
|wheel-macos-big-sur-cp39-arm64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815473848/jobs/6490473314)|
|wheel-macos-mojave-cp310-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815474407/jobs/6490474195)|
|wheel-macos-mojave-cp311-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815471445/jobs/6490469495)|
|wheel-macos-mojave-cp37-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815471579/jobs/6490469727)|
|wheel-macos-mojave-cp38-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815473111/jobs/6490472170)|
|wheel-macos-mojave-cp39-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815471860/jobs/6490470342)|
|wheel-manylinux2014-cp310-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815470531/jobs/6490468359)|
|wheel-manylinux2014-cp310-arm64|[](https://github.com/ursacomputing/crossbow/runs/10380975010)|
|wheel-manylinux2014-cp311-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815472878/jobs/6490471736)|
|wheel-manylinux2014-cp311-arm64|[](https://github.com/ursacomputing/crossbow/runs/10380976641)|
|wheel-manylinux2014-cp37-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815471751/jobs/6490470052)|
|wheel-manylinux2014-cp37-arm64|[](https://github.com/ursacomputing/crossbow/runs/10380975643)|
|wheel-manylinux2014-cp38-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815471648/jobs/6490469879)|
|wheel-manylinux2014-cp38-arm64|[](https://github.com/ursacomputing/crossbow/runs/10380980310)|
|wheel-manylinux2014-cp39-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815471795/jobs/6490470220)|
|wheel-manylinux2014-cp39-arm64|[](https://github.com/ursacomputing/crossbow/runs/10380980772)|
|wheel-windows-cp310-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815472772/jobs/6490471488)|
|wheel-windows-cp311-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815472103/jobs/6490470650)|
|wheel-windows-cp37-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815473510/jobs/6490472885)|
|wheel-windows-cp38-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815471072/jobs/6490468970)|
|wheel-windows-cp39-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/3815473315/jobs/6490472516)|
--
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] wjones127 commented on a diff in pull request #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
wjones127 commented on code in PR #15118:
URL: https://github.com/apache/arrow/pull/15118#discussion_r1059529100
##########
cpp/src/arrow/flight/transport/grpc/grpc_client.cc:
##########
@@ -499,6 +499,65 @@ constexpr char kDummyRootCert[] =
"-----END CERTIFICATE-----\n";
#endif
+class GrpcResultStream : public ResultStream {
+ public:
+ explicit GrpcResultStream(const FlightCallOptions& options)
+ : rpc_(options),
+ stop_token_(options.stop_token),
+ status_(
+ Status::UnknownError("Internal implementation error, stream not started")) {}
+
+ ~GrpcResultStream() override {
+ if (stream_) {
+ rpc_.context.TryCancel();
+ ARROW_WARN_NOT_OK(FromGrpcStatus(stream_->Finish(), &rpc_.context),
+ "DoAction result was not fully consumed");
+ }
+ }
+
+ static arrow::Result<std::unique_ptr<GrpcResultStream>> Make(
+ const FlightCallOptions& options, pb::FlightService::Stub* stub,
+ ClientAuthHandler* auth_handler, const Action& action) {
+ auto result = std::make_unique<GrpcResultStream>(options);
+ ARROW_RETURN_NOT_OK(result->Init(stub, auth_handler, action));
+ return result;
+ }
+
+ Status Init(pb::FlightService::Stub* stub, ClientAuthHandler* auth_handler,
+ const Action& action) {
+ pb::Action pb_action;
+ RETURN_NOT_OK(internal::ToProto(action, &pb_action));
+ RETURN_NOT_OK(rpc_.SetToken(auth_handler));
+ stream_ = stub->DoAction(&rpc_.context, pb_action);
+ return Status::OK();
+ }
+
+ arrow::Result<std::unique_ptr<Result>> Next() override {
+ if (stream_) {
+ pb::Result pb_result;
+ if (!stop_token_.IsStopRequested() && stream_->Read(&pb_result)) {
+ auto result = std::make_unique<Result>();
+ RETURN_NOT_OK(internal::FromProto(pb_result, result.get()));
+ return result;
+ } else if (stop_token_.IsStopRequested()) {
+ rpc_.context.TryCancel();
+ }
+ RETURN_NOT_OK(stop_token_.Poll());
+
+ status_ = FromGrpcStatus(stream_->Finish(), &rpc_.context);
+ stream_.reset();
+ }
+ RETURN_NOT_OK(status_);
+ return nullptr;
+ }
+
+ // private:
Review Comment:
Uncomment?
##########
cpp/src/arrow/flight/flight_test.cc:
##########
@@ -1286,11 +1289,12 @@ TEST_F(TestTls, OverrideHostname) {
ASSERT_OK_AND_ASSIGN(auto client, FlightClient::Connect(location_, client_options));
FlightCallOptions options;
- options.timeout = TimeoutDuration{5.0};
+ // options.timeout = TimeoutDuration{5.0};
Review Comment:
Should this be removed? Or kept?
--
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 #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #15118:
URL: https://github.com/apache/arrow/pull/15118#issuecomment-1368154230
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/17bdba7849264075adbdb3ce1fd3f4ee...04322a0af34b435d9031fdcf48a8779e/)
--
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 #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #15118:
URL: https://github.com/apache/arrow/pull/15118#issuecomment-1368543547
Or we need to make the new behavior opt-in somehow. (stream_action vs do_action?)
--
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 #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #15118:
URL: https://github.com/apache/arrow/pull/15118#issuecomment-1368544204
Or possibly, we can emulate the old behavior (ish) by having the client wait for initial metadata before returning. That way we know the server RPC handler has at least (partially) executed, but we haven't necessarily gotten a full response. (That won't be ideal for applications that do want streaming, but it would at least split the difference between the old and new behaviors.)
--
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] kou commented on pull request #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
kou commented on PR #15118:
URL: https://github.com/apache/arrow/pull/15118#issuecomment-1368358824
@github-actions crossbow submit -g wheel
--
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] wjones127 merged pull request #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
wjones127 merged PR #15118:
URL: https://github.com/apache/arrow/pull/15118
--
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 #15118: GH-15069: [C++][Python][FlightRPC] Make DoAction truly streaming
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #15118:
URL: https://github.com/apache/arrow/pull/15118#issuecomment-1368154198
Benchmark runs are scheduled for baseline = 498b645e1d09306bf5399a9a019a5caa99513815 and contender = 985789165eb30cc575d67aa52469f802e5132e64. 985789165eb30cc575d67aa52469f802e5132e64 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/5308ea72ebc34d54a40d6b81cecb091b...5c238f076209433497acdc495f3e8649/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/a9ed4360adca485880383ea1f73827e0...7f885578020847779fc23a7a26b4c100/)
[Finished :arrow_down:2.81% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/17bdba7849264075adbdb3ce1fd3f4ee...04322a0af34b435d9031fdcf48a8779e/)
[Failed :arrow_down:0.0% :arrow_up:0.0%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/ee0d29be90bf4692b4110637148bc3b2...bf5b4f81877b4ec383ded68a1604c478/)
Buildkite builds:
[Finished] [`98578916` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2086)
[Failed] [`98578916` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2108)
[Finished] [`98578916` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2080)
[Failed] [`98578916` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2100)
[Finished] [`498b645e` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2085)
[Failed] [`498b645e` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2107)
[Finished] [`498b645e` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2079)
[Failed] [`498b645e` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2099)
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