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

Fix remote client for HTTP transport #1411

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@trotterdylan
Contributor

trotterdylan commented Nov 7, 2017

This change fixes two issues:

  • Assign parsedUrl to the variable in the outer scope instead of creating a new one. Previously the outer parsedUrl was never assigned and was therefore always empty.
  • Create a POST client using NewTHttpPostClient instead of NewTHttpClient since the latter has no requestBuffer to write to the request body.
@jeking3

This comment has been minimized.

Show comment
Hide comment
@jeking3

jeking3 Nov 10, 2017

Contributor

He @trotterdylan please review https://thrift.apache.org/docs/HowToContribute as we need a Jira ticket for this change.

Contributor

jeking3 commented Nov 10, 2017

He @trotterdylan please review https://thrift.apache.org/docs/HowToContribute as we need a Jira ticket for this change.

@trotterdylan

This comment has been minimized.

Show comment
Hide comment
Contributor

trotterdylan commented Nov 13, 2017

@dcelasun

This comment has been minimized.

Show comment
Hide comment
@dcelasun

dcelasun Nov 13, 2017

Member

Please revert the NewTHttpClient part of this PR, since THttpPostClient is deprecated and is just an alias for THttpClient since 0dd8235.

Member

dcelasun commented Nov 13, 2017

Please revert the NewTHttpClient part of this PR, since THttpPostClient is deprecated and is just an alias for THttpClient since 0dd8235.

Fix remote client for HTTP transport
Assign parsedUrl to the variable in the outer scope instead of creating
a new one. Previously the outer parsedUrl was never assigned and was
therefore always empty.
@trotterdylan

This comment has been minimized.

Show comment
Hide comment
@trotterdylan

trotterdylan Nov 13, 2017

Contributor

Done. Updated the commit message accordingly.

Contributor

trotterdylan commented Nov 13, 2017

Done. Updated the commit message accordingly.

@jeking3

This comment has been minimized.

Show comment
Hide comment
@jeking3

jeking3 Nov 13, 2017

Contributor

Thanks - once the CI builds pass I will re-review.

Contributor

jeking3 commented Nov 13, 2017

Thanks - once the CI builds pass I will re-review.

@jeking3

This comment has been minimized.

Show comment
Hide comment
@jeking3

jeking3 Nov 18, 2017

Contributor

@dcelasun can you approve if these changes look good to you?

Contributor

jeking3 commented Nov 18, 2017

@dcelasun can you approve if these changes look good to you?

@dcelasun

This comment has been minimized.

Show comment
Hide comment
@dcelasun
Member

dcelasun commented Nov 20, 2017

@jeking3 LGTM

@asfgit asfgit closed this in cde4d41 Nov 20, 2017

jeking3 added a commit to jeking3/thrift that referenced this pull request Nov 30, 2017

Fix remote client for HTTP transport
Client: go

Assign parsedUrl to the variable in the outer scope instead of creating
a new one. Previously the outer parsedUrl was never assigned and was
therefore always empty.

This closes apache#1411
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment