[ 
https://issues.apache.org/jira/browse/GEODE-2109?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15712628#comment-15712628
 ] 

ASF GitHub Bot commented on GEODE-2109:
---------------------------------------

Github user kirklund commented on the issue:

    https://github.com/apache/incubator-geode/pull/296
  
    We've been requiring unit tests or integration tests for all bug fixes. 
    
    At a minimum, I would recommend adding a new test that fails due to at 
least of the conditions reported by GEODE-2109 and then passes with your 
changes. Example:
    `
    import static org.assertj.core.api.Assertions.assertThat; 
    
    import org.apache.geode.test.junit.categories.UnitTest;
    import org.junit.Rule;
    import org.junit.Test;
    import org.junit.contrib.java.lang.system.SystemErrRule;
    import org.junit.experimental.categories.Category;
    
    @Category(UnitTest.class)
    public class FederatingManagerSubmitTaskWithFailureTest {
    
      private FederatingManager manager;
    
      @Rule
      public SystemErrRule systemErrRule = new SystemErrRule().enableLog();
    
      @Test
      public void submittedTaskShouldLogFailure() throws Exception {
        manager.submitTask(() -> {throw new Exception("error message");});
        
assertThat(systemErrRule.getLog()).contains(Exception.class.getName()).contains("error
 message");
      }
    }
    `
    The above test would require a couple things:
    1) some sort of @Before method which instantiates manager
    2) change FederatingManager.submitTask(...) to package scope so the unit 
test can call it (right now FederatingManager is not a test-friendly class)



> calling submit on ExecutionService can cause exceptions to be lost
> ------------------------------------------------------------------
>
>                 Key: GEODE-2109
>                 URL: https://issues.apache.org/jira/browse/GEODE-2109
>             Project: Geode
>          Issue Type: Bug
>          Components: regions
>            Reporter: Darrel Schneider
>
> Geode has a number of places that call submit on ExecutionService. The submit 
> method returns a Future object. If the caller makes sure it calls "get" on 
> the Future then all is well. But in many places geode is not calling get. In 
> that case if the Runnable that was submitted throws an exception it gets 
> stored in the get and never logged. This can make it very hard to diagnose 
> problems.
> If the caller does not want to call get on the returned Future then it should 
> instead call the "execute" method. In that case the exception will be 
> unhandled and the unhandled exception handler code we have on the 
> LoggingThreadGroup class will cause the exception to be logged.
> Here are the places that should be changed to use execute instead of submit:
> org.apache.geode.internal.util.concurrent.CustomEntryConcurrentHashMap.clear()
> org.apache.geode.internal.cache.DiskStoreImpl.executeDiskStoreTask(Runnable)
> org.apache.geode.internal.cache.lru.HeapEvictor.onEvent(MemoryEvent)
> org.apache.geode.distributed.internal.InternalLocator.restartWithDS(InternalDistributedSystem,
>  GemFireCacheImpl)
> org.apache.geode.distributed.internal.FunctionExecutionPooledExecutor.FunctionExecutionPooledExecutor(BlockingQueue<Runnable>,
>  int, PoolStatHelper, ThreadFactory, int, boolean)
> org.apache.geode.internal.cache.PRHARedundancyProvider.scheduleCreateMissingBuckets()
> org.apache.geode.distributed.internal.InternalLocator.startSharedConfigurationService(GemFireCacheImpl)
> org.apache.geode.cache.client.internal.SingleHopClientExecutor.submitTask(Runnable)
> org.apache.geode.management.internal.FederatingManager.submitTask(Callable<DistributedMember>)



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to