geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Galen O'Sullivan <gosulli...@pivotal.io>
Subject Re: Review Request 59850: GEODE-3023: TcpServer thread can be blocked in processRequest
Date Fri, 09 Jun 2017 01:06:33 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59850/#review177427
-----------------------------------------------------------


Fix it, then Ship it!




I had a couple of minor comments about the tests, but otherwise it looks good.

Oh, and don't forget to run Spotless!


geode-core/src/test/java/org/apache/geode/distributed/internal/tcpserver/TCPServerSSLJUnitTest.java
Lines 82 (patched)
<https://reviews.apache.org/r/59850/#comment250962>

    I'm missing the time delay here. Can you show me where it gets delayed?
    
    ... ah, it looks like you copied this from `TcpServerJUnitTest`. Would you mind renaming
this function to something that makes more sense in this context?



geode-core/src/test/java/org/apache/geode/distributed/internal/tcpserver/TCPServerSSLJUnitTest.java
Lines 104 (patched)
<https://reviews.apache.org/r/59850/#comment250963>

    If we're expecting the exception as a requirement (and I think we are), we can assert
that it's true by setting a boolean to false and then setting it to true in the catch block.



geode-core/src/test/java/org/apache/geode/distributed/internal/tcpserver/TCPServerSSLJUnitTest.java
Lines 133 (patched)
<https://reviews.apache.org/r/59850/#comment250978>

    We can reasonably set a very small timeout here, even less than the timeout of the socket
(since we're mocking it). That (and setting the timeout to some silly number like 31337) would
also help us verify that we're mocking correctly.



geode-core/src/test/java/org/apache/geode/distributed/internal/tcpserver/TCPServerSSLJUnitTest.java
Lines 143 (patched)
<https://reviews.apache.org/r/59850/#comment250964>

    What's the GemStoneAddition about?


- Galen O'Sullivan


On June 8, 2017, 12:20 a.m., Udo Kohlmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59850/
> -----------------------------------------------------------
> 
> (Updated June 8, 2017, 12:20 a.m.)
> 
> 
> Review request for geode, Alexander Murmann, Bruce Schuchardt, Galen O'Sullivan, Hitesh
Khamesra, and Brian Rowe.
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> Moved the socket.setSoTimeout setting to be before the SSL handshake. This is to avoid
the timeout to never be set in the case of a SSLException. Added a test to test that the socket
timeout is correctly set upon failure within the SSL configuration and handshake.
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpServer.java
86fe53261 
>   geode-core/src/test/java/org/apache/geode/distributed/internal/tcpserver/TCPServerSSLJUnitTest.java
PRE-CREATION 
>   geode-core/src/test/java/org/apache/geode/distributed/internal/tcpserver/TcpServerJUnitTest.java
7c7a2b376 
>   geode-core/src/test/java/org/apache/geode/internal/net/DelaySocketCreator.java PRE-CREATION

> 
> 
> Diff: https://reviews.apache.org/r/59850/diff/2/
> 
> 
> Testing
> -------
> 
> Junit test - done
> precheckin - done
> 
> 
> Thanks,
> 
> Udo Kohlmeyer
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message