[GitHub] asdf2014 commented on issue #6090: Fix missing exception handling as part of `io.druid.java.util.http.client.netty.HttpClientPipelineFactory`

[GitBox]

asdf2014 commented on issue #6090: Fix missing exception handling as part of 
`io.druid.java.util.http.client.netty.HttpClientPipelineFactory`
URL: https://github.com/apache/incubator-druid/pull/6090#issuecomment-410170672
 
 
   BTW, when i tried to use `ExpectedException` instead of `Assert.assertTrue` 
in `JankyServersTest` for other test cases, i realized the 
`JankyServersTest#isChannelClosedException` situation was hard to convert to 
`ExpectedException` way, so i created a sub-calss of `TypeSafeMatcher` to 
resolve the problem.
   
   ```java
   private static class CauseMatcher extends TypeSafeMatcher<Throwable>
   {
     private final Class<? extends Throwable> expectedType;
     private final String expectedMessage;
     private final boolean isRegex;
   
     public CauseMatcher(Class<? extends Throwable> expectedType, String 
expectedMessage)
     {
       this.expectedType = expectedType;
       this.expectedMessage = expectedMessage;
       this.isRegex = false;
     }
   
     public CauseMatcher(Class<? extends Throwable> expectedType, String 
expectedMessage, boolean isRegex)
     {
       this.expectedType = expectedType;
       this.expectedMessage = expectedMessage;
       this.isRegex = isRegex;
     }
   
     @Override
     protected boolean matchesSafely(Throwable item)
     {
       if (item == null || item.getClass() == null || item.getMessage() == 
null) {
         return false;
       }
       if (!item.getClass().isAssignableFrom(expectedType)) {
         return false;
       }
       if (isRegex) {
         return 
Pattern.compile(expectedMessage).matcher(item.getMessage()).find();
       } else {
         return item.getMessage().contains(expectedMessage);
       }
     }
   
     @Override
     public void describeTo(Description description)
     {
       description.appendText("expects type is ")
           .appendValue(expectedType)
           .appendText(" and message is ")
           .appendValue(expectedMessage);
     }
   }
   ```
   
   Then, use the following code to express the 
`JankyServersTest#isChannelClosedException` logic.
   
   ```java
   expectedException.expectCause(
       anyOf(
           new CauseMatcher(ChannelException.class, "Faulty channel in resource 
pool"),
           new CauseMatcher(IOException.class, ".*Connection reset by peer.*", 
true)
       )
   );
   ```
   
   However, this change will add too many new lines of code. If just to solve 
this single situation would be a huge cost, but it can be used as a common util 
class for other test cases, then it might be worth adding. What do you think? 
@jihoonson

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@druid.apache.org
For additional commands, e-mail: commits-h...@druid.apache.org