You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by deepakddixit <gi...@git.apache.org> on 2016/11/26 04:11:57 UTC
[GitHub] incubator-geode pull request #296: GEODE-2109 : Calling submit on ExecutionS...
GitHub user deepakddixit opened a pull request:
https://github.com/apache/incubator-geode/pull/296
GEODE-2109 : Calling submit on ExecutionService can cause exceptions to be lost
Replaced submit call with execute.
Refactored GIITask calls from DiskStoreImpl. Async task like Creating KRF's through disk store are replaced with execute. The other call is kept with submit as we later check for whether delayed write is completed or not.
Added RejectedExecutionException to IgnoredException list as from these tests it may get occasionally thrown and as we moved to execute method of ExecutorService they will be logged.
All precheckin tests are passing except one org.apache.geode.management.internal.cli.commands.IndexCommandsDUnitTest.testCreateDestroyUpdatesSharedConfig which is marked as Flaky.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/deepakddixit/incubator-geode bugfix/GEODE-2109
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-geode/pull/296.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #296
----
commit 8c4aeaae23d242b9a4edfe328bd4046e5626914d
Author: Deepak Dixit <de...@ampool.io>
Date: 2016-11-23T16:03:14Z
GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call.
This will help in logging exceptions those come in the thread.
commit db213bb1d0982db12ffc5e8d2649cec8f69147c6
Author: Deepak Dixit <de...@ampool.io>
Date: 2016-11-24T06:27:58Z
GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask.
Refactored DiskStore task for delayed writes, we cannot replace expensive writes tasks submit call with execute as later we check if write call is completed or not.
Replaced submit call by execute in case of DiskStore tasks like compactions, creating KRF's.
commit 478a714979ace2f187ebf336c0d5121a0a5c8940
Author: Deepak Dixit <de...@ampool.io>
Date: 2016-11-24T12:21:54Z
GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask.
Replaced submit call for GIITask and RemoveMember tasks. GIITask is used when adding member or starting managing activity when node becomes managing node. While adding member we can make the ExecutorService.execute call. Not changed call for GIITask when invoked from managing activity as possible exception is handled.
commit b501654a3718f3e15cc45b9bd757bfb558f664a1
Author: Deepak Dixit <de...@ampool.io>
Date: 2016-11-24T16:27:38Z
GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask.
Ignoring rejectedException as this can be thrown in case pool is shutting down.
commit 9e56037ac21768adde9c35c97bc8e0cb463042b7
Author: Deepak Dixit <de...@ampool.io>
Date: 2016-11-24T17:17:53Z
GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask.
Changing way creating GIITask when node becomes managing node.
commit 3b8273b7746f5b4af329c92c92fac07e6571cf05
Author: Deepak Dixit <de...@ampool.io>
Date: 2016-11-25T05:15:26Z
GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask.
Adding ignoredException for MemoryThresholdsDUnitTest.
commit 5cf0aac229c595b91436c4d327d43444120698b5
Author: Deepak Dixit <de...@ampool.io>
Date: 2016-11-25T08:05:39Z
GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask.
Adding ignoredException for EvictionStatsDUnitTest.
commit ed24011d0b1456b0247bd0254af252cae351eb07
Author: Deepak Dixit <de...@ampool.io>
Date: 2016-11-26T04:02:26Z
GEODE-2109: Replacing ExecutorService.submit calls with ExecutorService.execute call for DiskStoreTask.
Applying spotless checks.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode issue #296: GEODE-2109 : Calling submit on ExecutionService ...
Posted by deepakddixit <gi...@git.apache.org>.
Github user deepakddixit commented on the issue:
https://github.com/apache/incubator-geode/pull/296
@upthewaterspout Thanks for the review. The class declaration is mistakenly changed. I have corrected it. All tests are passing from precheckin after this.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode issue #296: GEODE-2109 : Calling submit on ExecutionService ...
Posted by kirklund <gi...@git.apache.org>.
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)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode issue #296: GEODE-2109 : Calling submit on ExecutionService ...
Posted by deepakddixit <gi...@git.apache.org>.
Github user deepakddixit commented on the issue:
https://github.com/apache/incubator-geode/pull/296
@kirklund I have added a test as suggested with SingleHopTestExecutor. Kindly review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-geode pull request #296: GEODE-2109 : Calling submit on ExecutionS...
Posted by upthewaterspout <gi...@git.apache.org>.
Github user upthewaterspout commented on a diff in the pull request:
https://github.com/apache/incubator-geode/pull/296#discussion_r90118721
--- Diff: geode-core/src/main/java/org/apache/geode/management/internal/FederatingManager.java ---
@@ -386,7 +398,7 @@ public void startManagingActivity() throws Exception {
*
*/
- private class GIITask implements Callable<DistributedMember> {
+ private class GIITask implements Callable {
--- End diff --
Why did you remove <DistributedMember> from this class declaration?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---