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

ARROW-8183: [C++][Python][FlightRPC] Expose transport error metadata #6747

Closed
wants to merge 1 commit into from

Conversation

rymurr
Copy link
Contributor

@rymurr rymurr commented Mar 28, 2020

This is the C++ and Python version of ARROW-8181

  • c++
  • python

@github-actions
Copy link

@lidavidm lidavidm self-requested a review March 29, 2020 14:14
@rymurr rymurr changed the title ARROW-8183: [WIP][C++][Python][FlightRPC] Expose transport error metadata ARROW-8183: [C++][Python][FlightRPC] Expose transport error metadata Mar 29, 2020
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks for following up with this. My main comment is about not losing the underlying gRPC error information on the C++ side by not overriding the status unconditionally. This will also fix the tests.

cpp/src/arrow/flight/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/src/arrow/flight/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/src/arrow/flight/internal.cc Outdated Show resolved Hide resolved
*status = Status(code, message);
const auto grpc_detail_val = trailers.find(kBinaryErrorDetailsKey);
std::shared_ptr<FlightStatusDetail> flightStatusDetail(
new FlightStatusDetail(FlightStatusCode::Unavailable));
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't use Unavailable. We should probably add a new enum variant for when there's no other applicable code, or otherwise default to INTERNAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in most recent path

cpp/src/arrow/flight/internal.cc Outdated Show resolved Hide resolved
cpp/src/arrow/flight/internal.cc Outdated Show resolved Hide resolved
cpp/src/arrow/flight/types.cc Outdated Show resolved Hide resolved
python/pyarrow/_flight.pyx Outdated Show resolved Hide resolved
python/pyarrow/_flight.pyx Outdated Show resolved Hide resolved
python/pyarrow/_flight.pyx Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@rymurr
Copy link
Contributor Author

rymurr commented Mar 29, 2020

Thanks a lot for the comments @lidavidm! will address and get back to you.

@rymurr rymurr force-pushed the ARROW-8183 branch 3 times, most recently from fa50795 to 5b740b3 Compare March 30, 2020 10:18
This is the C++ and Python version of ARROW-8181
Copy link
Member

@lidavidm lidavidm left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants