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

THRIFT-4914: Add THeader to context object for server reads #1840

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

fishy
Copy link
Member

@fishy fishy commented Aug 2, 2019

Client: go

This is the first part of THRIFT-4914, which handles the server reading
part in the requests (client -> server direction).

In TSimpleServer implementation, when the protocol is THeaderProtocol,
auto add all the headers read into the context object before passing
that to processor, so the processor code can access them from the
context object directly, via the new helper functions added in
header_context.go file.

  • Did you create an Apache Jira ticket? (not required for trivial changes)
  • Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"? (not required for trivial changes)
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, add [skip ci] at the end of your pull request to free up build resources.

)

// SetContextHeader sets a header in the context.
func SetContextHeader(ctx context.Context, key, value string) context.Context {
Copy link
Member

Choose a reason for hiding this comment

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

SetHeader sounds better, no need to mention Context in the function name. Same goes for all the other functions in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dcelasun My main concern is that we have functions sound similar, like thrift.(*THeaderProtocol).SetWriteHeader. Although there's no conflicts this might look confusing and making it more explicit might be better. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it in a fixup commit. If we decided that we do want the rename I'll squash the branch.

Copy link
Member

Choose a reason for hiding this comment

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

Since the other functions belong to THeaderProtocol and these new ones are on the package level, I don't think there'll be much confusion.

If you have any other changes you'd like to do, please go ahead. Otherwise I'm gonna squash & merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

I squashed them. Thanks for the review!

Client: go

This is the first part of THRIFT-4914, which handles the server reading
part in the requests (client -> server direction).

In TSimpleServer implementation, when the protocol is THeaderProtocol,
auto add all the headers read into the context object before passing
that to processor, so the processor code can access them from the
context object directly, via the new helper functions added in
header_context.go file.
@fishy fishy force-pushed the theader-context-server-read branch from ac643f1 to 1ff225d Compare August 5, 2019 19:23
@dcelasun dcelasun merged commit b1002a7 into apache:master Aug 5, 2019
@fishy fishy deleted the theader-context-server-read branch August 5, 2019 20:08
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