Skip to content

ARROW-16069: [C++][FlightRPC] Refactor out gRPC error code handling - #12749

Closed
lidavidm wants to merge 7 commits into
apache:masterfrom
lidavidm:arrow-16069
Closed

ARROW-16069: [C++][FlightRPC] Refactor out gRPC error code handling#12749
lidavidm wants to merge 7 commits into
apache:masterfrom
lidavidm:arrow-16069

Conversation

@lidavidm

Copy link
Copy Markdown
Member

No description provided.

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@lidavidm

lidavidm commented Mar 29, 2022

Copy link
Copy Markdown
MemberAuthor

TODOs:

@lidavidmlidavidm changed the title ARROW-16069: [C++][FlightRPC] WIP: Refactor out gRPC error code handlingARROW-16069: [C++][FlightRPC] Refactor out gRPC error code handlingApr 8, 2022
@lidavidm
lidavidm marked this pull request as ready for review April 11, 2022 12:58
@lidavidm

Copy link
Copy Markdown
MemberAuthor

CC @cyb70289, this addresses the error handling questions from the last PR by refactoring out the gRPC implementation for UCX to use.

Comment threadcpp/src/arrow/flight/transport.cc
@ursabot

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 62d0b17 and contender = fc9af3c. fc9af3c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️0.13% ⬆️0.0%] test-mac-arm
[Finished ⬇️0.36% ⬆️0.36%] ursa-i9-9960x
[Finished ⬇️0.34% ⬆️0.13%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/507|fc9af3cd ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/494|fc9af3cd test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/493|fc9af3cd ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/504|fc9af3cd ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/506|62d0b179 ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/493|62d0b179 test-mac-arm>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/492|62d0b179 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/503|62d0b179 ursa-thinkcentre-m75q>
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

@kou

kou commented Apr 15, 2022

Copy link
Copy Markdown
Member

@lidavidm It seems that this breaks CI on macOS:

https://github.com/ursacomputing/crossbow/runs/6038992284?check_suite_focus=true#step:6:5601

[ 64%] Building CXX object src/arrow/flight/CMakeFiles/arrow_flight_testing_objlib.dir/test_definitions.cc.o
/Users/runner/work/crossbow/crossbow/arrow/cpp/src/arrow/flight/test_definitions.cc:1370:45: error: constexpr variable cannot have non-literal type 'const std::initializer_list<StatusCode>'
constexpr std::initializer_list<StatusCode> kStatusCodes = {
^
/Applications/Xcode_12.4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/initializer_list:58:28: note: 'initializer_list<arrow::StatusCode>' is not literal because it is not an aggregate and has no constexpr constructors other than copy or move constructors
class _LIBCPP_TEMPLATE_VIS initializer_list
^
/Users/runner/work/crossbow/crossbow/arrow/cpp/src/arrow/flight/test_definitions.cc:1389:51: error: constexpr variable cannot have non-literal type 'const std::initializer_list<FlightStatusCode>'
constexpr std::initializer_list<FlightStatusCode> kFlightStatusCodes = {
^
/Applications/Xcode_12.4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/initializer_list:58:28: note: 'initializer_list<arrow::flight::FlightStatusCode>' is not literal because it is not an aggregate and has no constexpr constructors other than copy or move constructors
class _LIBCPP_TEMPLATE_VIS initializer_list
^
2 errors generated.

Could you confirm this?

@lidavidm

Copy link
Copy Markdown
MemberAuthor

I can't reproduce it right away with Clang 12 locally (on Linux), but regardless we can fix that. Thanks for catching this Kou. See #12903.

@kou

kou commented Apr 16, 2022

Copy link
Copy Markdown
Member

@github-actions crossbow submit python-sdist

@github-actions

Copy link
Copy Markdown

Revision: a00113b

Submitted crossbow builds: ursacomputing/crossbow @ actions-1867

TaskStatus
python-sdistGithub Actions

"""A Flight server that uses all the Flight-specific errors."""

errors = {
"internal": flight.FlightInternalError,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lidavidmflight is None when we don't enable Flight.

See also: https://github.com/ursacomputing/crossbow/runs/6046421802?check_suite_focus=true#step:6:2372

==================================== ERRORS ====================================
_ ERROR collecting usr/local/lib/python3.8/dist-packages/pyarrow/tests/test_flight.py _
usr/local/lib/python3.8/dist-packages/pyarrow/tests/test_flight.py:355: in <module>
class ErrorFlightServer(FlightServerBase):
usr/local/lib/python3.8/dist-packages/pyarrow/tests/test_flight.py:359: in ErrorFlightServer
"internal": flight.FlightInternalError,
E AttributeError: 'NoneType' object has no attribute 'FlightInternalError'

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that. See #12911/ARROW-16216.

Sign up for freeto 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.

4 participants

@lidavidm@ursabot@kou@cyb70289