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

Merge trailer and inital headers into status on err #510

Merged
merged 1 commit into from
Dec 14, 2020

Conversation

shepmaster
Copy link
Contributor

Motivation

The C++ gRPC server sometimes returns both headers and trailers. An
excerpt from Wireshark:

Stream: HEADERS, Stream ID: 1, Length 136, 200 OK
    Header: :status: 200 OK
    Header: x-middleware: expected value
    Header: content-type: application/grpc
    Header: grpc-accept-encoding: identity,deflate,gzip
    Header: accept-encoding: identity,gzip

Stream: HEADERS, Stream ID: 1, Length 92
    Header: grpc-status: 2
    Header: grpc-message: Unknown
    Header: x-arrow-status: 9
    Header: x-arrow-status-message-bin: VW5rbm93bg

Before this commit, only the metadata from the trailer would be
available, missing the x-middleware header:

MetadataMap {
    headers: {
        "x-arrow-status-message-bin": "VW5rbm93bg",
        "x-arrow-status": "9",
    },
}

Solution

Merge the metadata before returning the error

The C++ gRPC server sometimes returns both headers and trailers. An
excerpt from Wireshark:

```
Stream: HEADERS, Stream ID: 1, Length 136, 200 OK
    Header: :status: 200 OK
    Header: x-middleware: expected value
    Header: content-type: application/grpc
    Header: grpc-accept-encoding: identity,deflate,gzip
    Header: accept-encoding: identity,gzip

Stream: HEADERS, Stream ID: 1, Length 92
    Header: grpc-status: 2
    Header: grpc-message: Unknown
    Header: x-arrow-status: 9
    Header: x-arrow-status-message-bin: VW5rbm93bg
```

Before this commit, only the metadata from the trailer would be
available, missing the `x-middleware` header:

```
MetadataMap {
    headers: {
        "x-arrow-status-message-bin": "VW5rbm93bg",
        "x-arrow-status": "9",
    },
}
```
@shepmaster
Copy link
Contributor Author

Talking with @LucioFranco in Discord, it seems like it's very hard (impossible?) to cause Tonic to perform the same behavior of sending separate headers and trailers, so it's not clear how best to write a test for this at this point.

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Thanks!

@LucioFranco LucioFranco changed the title Merge metadata from headers with the trailers when there's an error Merge trailer and inital headers into status on err Dec 14, 2020
@LucioFranco LucioFranco merged commit f58fcf3 into hyperium:master Dec 14, 2020
nevi-me pushed a commit to apache/arrow that referenced this pull request Jan 9, 2021
This PR has a few refactorings and then the main commit contains a new Flight integration test client and server 🎉

The middleware scenario tests are currently skipped because they will fail until `tonic` can be updated to a version containing [a fix having to do with trailers](hyperium/tonic#510); this is tracked in [ARROW-10961](https://issues.apache.org/jira/browse/ARROW-10961).

Some Rust <-> Java integration tests will fail until [this PR is merged](#8963); I'm happy to rebase once that goes in, but I wanted to get code review started on this. Thank you!!

Closes #9049 from carols10cents/rust-flight-integration

Lead-authored-by: Carol (Nichols || Goulding) <carol.nichols@gmail.com>
Co-authored-by: Jake Goulding <jake.goulding@gmail.com>
Signed-off-by: Neville Dipale <nevilledips@gmail.com>
alamb pushed a commit to apache/arrow-rs that referenced this pull request Apr 20, 2021
This PR has a few refactorings and then the main commit contains a new Flight integration test client and server 🎉

The middleware scenario tests are currently skipped because they will fail until `tonic` can be updated to a version containing [a fix having to do with trailers](hyperium/tonic#510); this is tracked in [ARROW-10961](https://issues.apache.org/jira/browse/ARROW-10961).

Some Rust <-> Java integration tests will fail until [this PR is merged](apache/arrow#8963); I'm happy to rebase once that goes in, but I wanted to get code review started on this. Thank you!!

Closes #9049 from carols10cents/rust-flight-integration

Lead-authored-by: Carol (Nichols || Goulding) <carol.nichols@gmail.com>
Co-authored-by: Jake Goulding <jake.goulding@gmail.com>
Signed-off-by: Neville Dipale <nevilledips@gmail.com>
GeorgeAp pushed a commit to sirensolutions/arrow that referenced this pull request Jun 7, 2021
This PR has a few refactorings and then the main commit contains a new Flight integration test client and server 🎉

The middleware scenario tests are currently skipped because they will fail until `tonic` can be updated to a version containing [a fix having to do with trailers](hyperium/tonic#510); this is tracked in [ARROW-10961](https://issues.apache.org/jira/browse/ARROW-10961).

Some Rust <-> Java integration tests will fail until [this PR is merged](apache#8963); I'm happy to rebase once that goes in, but I wanted to get code review started on this. Thank you!!

Closes apache#9049 from carols10cents/rust-flight-integration

Lead-authored-by: Carol (Nichols || Goulding) <carol.nichols@gmail.com>
Co-authored-by: Jake Goulding <jake.goulding@gmail.com>
Signed-off-by: Neville Dipale <nevilledips@gmail.com>
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.

2 participants