[
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)
