You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by GitBox <gi...@apache.org> on 2022/08/22 20:57:39 UTC
[GitHub] [thrift] nithinkdb opened a new pull request, #2645: Added conditional support in xhr
nithinkdb opened a new pull request, #2645:
URL: https://github.com/apache/thrift/pull/2645
<!-- Explain the changes in the pull request below: -->
This is a continuation of a pull request that was created a while ago. I've replicated the changes, and added an additional check that keeps functionality largely the same while enabling TBinaryProtocol to be used in XHRRequests. This is needed for functionality in one of our thrift projects.
Link to original PR:
https://github.com/apache/thrift/pull/2157
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? (not required for trivial changes)
- [ ] If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
- [ ] 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, include `[skip ci]` anywhere in the commit message to free up build resources.
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] nithinkdb commented on pull request #2645: Added conditional support in xhr
Posted by GitBox <gi...@apache.org>.
nithinkdb commented on PR #2645:
URL: https://github.com/apache/thrift/pull/2645#issuecomment-1276639109
Hi, any updates on this? Has anyone from the Apache Thrift team taken a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] moderakh commented on a diff in pull request #2645: Added conditional support in xhr
Posted by GitBox <gi...@apache.org>.
moderakh commented on code in PR #2645:
URL: https://github.com/apache/thrift/pull/2645#discussion_r955296669
##########
lib/nodejs/lib/thrift/xhr_connection.js:
##########
@@ -102,13 +103,22 @@ XHRConnection.prototype.flush = function() {
var xreq = this.getXmlHttpRequestObject();
+ if (this.protocol == TBinaryProtocol) {
Review Comment:
which binary protocol is this referring to?
databricks thrift uses Http Thrift protocol not Binary thrift protocol. Is this referring to the same conectp?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] Jens-G commented on a diff in pull request #2645: Added conditional support in xhr
Posted by GitBox <gi...@apache.org>.
Jens-G commented on code in PR #2645:
URL: https://github.com/apache/thrift/pull/2645#discussion_r957867273
##########
lib/nodejs/lib/thrift/xhr_connection.js:
##########
@@ -102,13 +103,22 @@ XHRConnection.prototype.flush = function() {
var xreq = this.getXmlHttpRequestObject();
+ if (this.protocol == TBinaryProtocol) {
Review Comment:
For the sake of [terminology](https://thrift.apache.org/docs/concepts.html): HTTP is a **transport** which can of course be combined with a binary **protocol**.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] nithinkdb commented on pull request #2645: Added conditional support in xhr
Posted by GitBox <gi...@apache.org>.
nithinkdb commented on PR #2645:
URL: https://github.com/apache/thrift/pull/2645#issuecomment-1223213249
Looks like test flakiness is the reason the build failed (not my changes).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [thrift] nithinkdb commented on a diff in pull request #2645: Added conditional support in xhr
Posted by GitBox <gi...@apache.org>.
nithinkdb commented on code in PR #2645:
URL: https://github.com/apache/thrift/pull/2645#discussion_r957602390
##########
lib/nodejs/lib/thrift/xhr_connection.js:
##########
@@ -102,13 +103,22 @@ XHRConnection.prototype.flush = function() {
var xreq = this.getXmlHttpRequestObject();
+ if (this.protocol == TBinaryProtocol) {
Review Comment:
Ya, each connection within thrift has an associated protocol for encoding as well as a transport type. We use an HTTP connection within the nodeconnector, and we encode that information with a binary protocol and send it in chunks (bufffered transport).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org