Skip to content

ARROW-8112: [FlightRPC][C++] make sure status codes round-trip through gRPC - #6615

Closed
lidavidm wants to merge 3 commits into
apache:masterfrom
lidavidm:flight-missing-codes
Closed

ARROW-8112: [FlightRPC][C++] make sure status codes round-trip through gRPC#6615
lidavidm wants to merge 3 commits into
apache:masterfrom
lidavidm:flight-missing-codes

Conversation

@lidavidm

@lidavidmlidavidm commented Mar 13, 2020

Copy link
Copy Markdown
Member

There are still unmapped status codes, but these are the ones that correspond closely to a gRPC one. (OutOfMemory, for instance, doesn't quite line up with RESOURCE_EXHAUSTED since the latter is intended for some application-level resource like a disk quota, not an internal server error.)

@github-actions

github-actionsBot commented Mar 13, 2020

Copy link
Copy Markdown

https://issues.apache.org/jira/browse/ARROW-8112

@lidavidm

lidavidm commented Mar 13, 2020

Copy link
Copy Markdown
MemberAuthor

Python is failing and would be fixed by #6614

@lidavidm
lidavidm requested a review from pitrouMarch 13, 2020 14:59
@lidavidm

lidavidm commented Mar 13, 2020

Copy link
Copy Markdown
MemberAuthor

@pitrou I implemented the headers idea you suggested.

lidavidm
lidavidm commented Mar 13, 2020
View reviewed changes
Comment threadcpp/src/arrow/flight/internal.cc Outdated

@lidavidmlidavidmMar 13, 2020

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.

Is this worth refactoring out into status.h?

@lidavidmlidavidmMar 13, 2020

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.

(this being the int<->statuscode mapping)

@pitroupitrouMar 16, 2020

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.

Hmm... perhaps as a helper function?

@lidavidmlidavidmMar 16, 2020

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.

I've made a helper function, but kept it internal, since there doesn't seem to be a need to serialize statuses outside of Flight.

@lidavidm
lidavidmforce-pushed the flight-missing-codes branch from 2dd7c89 to d86276cCompareMarch 13, 2020 16:54
@lidavidm

lidavidm commented Mar 13, 2020

Copy link
Copy Markdown
MemberAuthor

This is segfaulting in Python tests, taking a look...

@lidavidm
lidavidmforce-pushed the flight-missing-codes branch 2 times, most recently from f3fe0db to 2b643fdCompareMarch 14, 2020 02:59
pitrou
pitrou requested changes Mar 16, 2020
View reviewed changes

@pitroupitrou left a comment

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.

Thank you! This will be a useful improvement.

Comment threadcpp/src/arrow/flight/internal.cc Outdated
Comment threadcpp/src/arrow/flight/internal.cc Outdated

@pitroupitrouMar 16, 2020

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.

Hmm... perhaps as a helper function?

Comment threadcpp/src/arrow/flight/internal.cc Outdated
Comment threadcpp/src/arrow/flight/internal.cc Outdated
Comment threadcpp/src/arrow/flight/internal.cc Outdated
Comment threadcpp/src/arrow/flight/server.cc Outdated
@lidavidm
lidavidmforce-pushed the flight-missing-codes branch from 2b643fd to 2af6537CompareMarch 16, 2020 13:08
@pitrou
pitrouforce-pushed the flight-missing-codes branch from 2af6537 to da2b346CompareMarch 17, 2020 12:01
pitrou
pitrou approved these changes Mar 17, 2020
View reviewed changes

@pitroupitrou left a comment

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.

+1. Will merge if green.

@pitrou

pitrou commented Mar 17, 2020

Copy link
Copy Markdown
Member

Thank you @lidavidm !

@asfimportasfimport mentioned this pull request Mar 17, 2020
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pitroupitroupitrou approved these changes

Assignees

No one assigned

Labels

Component: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lidavidm@pitrou