You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/08/26 20:54:12 UTC
[GitHub] [calcite-avatica] joshelser opened a new pull request #127: CALCITE-4196 Consume all data from client before replying with HTTP…
joshelser opened a new pull request #127:
URL: https://github.com/apache/calcite-avatica/pull/127
…/401
SPNEGO's handshake involves sending an HTTP/401 to "challenge" the
client to reply with authentication data. If the client is sending
a significant amount of data in the original request, the client
will still be writing this data when the server replies. This causes
the client to receive a TCP Reset when it continues to write data, and
ultimately manifests in a "Broken Pipe" runtime exception.
The fix is to simply consume all data the client wrote prior to
responding with the HTTP/401.
This also changed some stuff in AvaticaProtobufHandler to reduce a level of nested conditionals and fix a Logger class. The overall flow/purpose of that class did not change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] joshelser commented on pull request #127: CALCITE-4196 Consume all data from client before replying with HTTP…
Posted by GitBox <gi...@apache.org>.
joshelser commented on pull request #127:
URL: https://github.com/apache/calcite-avatica/pull/127#issuecomment-681127290
Forgot to mention -- I tried to write a unit test which demonstrates the 401 and the broken pipe, but I wasn't able to. My only guess is that going against localhost/loopback instead of over the network was avoiding the issue somehow. Didn't seem worth it to figure out why that didn't work when I had reproduced the issue with Apache Phoenix and was able to validate that this fix works.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] joshelser commented on a change in pull request #127: CALCITE-4196 Consume all data from client before replying with HTTP…
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #127:
URL: https://github.com/apache/calcite-avatica/pull/127#discussion_r478593632
##########
File path: .gitignore
##########
@@ -22,3 +22,4 @@ settings.xml
/target/
/*/target/
/shaded/*/target/
+bin
Review comment:
Hrm, I did not. I don't think we have such folders in avatica, but that's not great either. Let me revert that.
Thanks @vlsi
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] vlsi commented on a change in pull request #127: CALCITE-4196 Consume all data from client before replying with HTTP…
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #127:
URL: https://github.com/apache/calcite-avatica/pull/127#discussion_r477604972
##########
File path: .gitignore
##########
@@ -22,3 +22,4 @@ settings.xml
/target/
/*/target/
/shaded/*/target/
+bin
Review comment:
This would ignore all bin folders in all subfolders (e.g. it would ignore `org.apache.calcite.encodings.bin....` folder if one is created). Do you intend that?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] joshelser commented on a change in pull request #127: CALCITE-4196 Consume all data from client before replying with HTTP…
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #127:
URL: https://github.com/apache/calcite-avatica/pull/127#discussion_r477593904
##########
File path: server/src/main/java/org/apache/calcite/avatica/server/AbstractAvaticaHandler.java
##########
@@ -34,6 +38,7 @@
*/
public abstract class AbstractAvaticaHandler extends AbstractHandler
implements MetricsAwareAvaticaHandler {
+ private static final Logger LOG = LoggerFactory.getLogger(AbstractAvaticaHandler.class);
Review comment:
ah, yup. I had some logging in here as a part of my work, but considered none of it beneficial long-term.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] joshelser closed pull request #127: CALCITE-4196 Consume all data from client before replying with HTTP…
Posted by GitBox <gi...@apache.org>.
joshelser closed pull request #127:
URL: https://github.com/apache/calcite-avatica/pull/127
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite-avatica] risdenk commented on a change in pull request #127: CALCITE-4196 Consume all data from client before replying with HTTP…
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #127:
URL: https://github.com/apache/calcite-avatica/pull/127#discussion_r477590773
##########
File path: server/src/main/java/org/apache/calcite/avatica/server/AbstractAvaticaHandler.java
##########
@@ -34,6 +38,7 @@
*/
public abstract class AbstractAvaticaHandler extends AbstractHandler
implements MetricsAwareAvaticaHandler {
+ private static final Logger LOG = LoggerFactory.getLogger(AbstractAvaticaHandler.class);
Review comment:
nit: Looks like LOG is potentially unused?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org