You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by pivotal-jbarrett <gi...@git.apache.org> on 2017/03/20 14:37:56 UTC
[GitHub] geode-native pull request #63: GEODE-2657
GitHub user pivotal-jbarrett opened a pull request:
https://github.com/apache/geode-native/pull/63
GEODE-2657
Please make a quick review.
Fixes all the integration tests around Function Execution and adds test of AppDomain Function Execution.
Test testThinClientPoolExecuteFunctionDisableChunkHandlerThread is split out of testThinClientPoolExecuteFunction and marked OMITTED until GEODE-2691 is addressed.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pivotal-jbarrett/geode-native feature/GEODE-2657
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/geode-native/pull/63.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 #63
----
commit a0bc51ae1026097f525b99c0141c6eb73b3f5714
Author: Jacob Barrett <jb...@pivotal.io>
Date: 2017-03-17T23:38:32Z
GEODE-2657: Fixes and tests Function Execution.
- Corrects message formatting.
- Fixes integration tests.
commit cb02af7ec480ec6a46ea1c6fefc16f1697b33bf3
Author: Jacob Barrett <jb...@pivotal.io>
Date: 2017-03-16T13:36:32Z
GEODE-2657: Fixes .NET test ThinClientFunctionExecutionTests.
- Timeout is ms.
commit 9869b783c56119fa30f3e6712180a52e2398d52d
Author: Jacob Barrett <jb...@pivotal.io>
Date: 2017-03-18T00:21:20Z
GEODE-2657: Tests Function Execution with AppDomains.
----
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106930126
--- Diff: src/cppcache/integration-test/testThinClientPoolExecuteFunction.cpp ---
@@ -1506,48 +1353,10 @@ void runFunctionExecution(bool isEndpoint) {
CALL_TASK(StopC1);
CALL_TASK(CloseServers);
CALL_TASK(CloseLocator1);
-
--- End diff --
Like I said in the review request. This test was split out into tho two tests. The test had two different tests crammed into one file. The big delete in this file is all the crap that became the second test.
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106923938
--- Diff: src/cppcache/integration-test/CMakeLists.txt ---
@@ -169,8 +169,6 @@ set_property(TEST testThinClientPartitionResolver PROPERTY LABELS FLAKY)
set_property(TEST testThinClientPdxDeltaWithNotification PROPERTY LABELS FLAKY)
set_property(TEST testThinClientPdxInstance PROPERTY LABELS FLAKY)
set_property(TEST testThinClientPdxTests PROPERTY LABELS FLAKY)
--- End diff --
5 more live tests, that is nice
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by PivotalSarge <gi...@git.apache.org>.
Github user PivotalSarge commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106922752
--- Diff: src/clicache/src/CacheFactory.cpp ---
@@ -84,6 +85,12 @@ namespace Apache
//TODO::split
SafeConvertClassGeneric::SetAppDomainEnabled(appDomainEnable);
+ if (appDomainEnable)
+ {
+ // Register managed AppDomain context with unmanaged.
+ apache::geode::client::createAppDomainContext = &Apache::Geode::Client::createAppDomainContext;
--- End diff --
Does moving the registration of the managed app domain context to cache factory from distributed system make it available sooner?
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106927884
--- Diff: src/clicache/src/CacheFactory.cpp ---
@@ -84,6 +85,12 @@ namespace Apache
//TODO::split
SafeConvertClassGeneric::SetAppDomainEnabled(appDomainEnable);
+ if (appDomainEnable)
+ {
+ // Register managed AppDomain context with unmanaged.
+ apache::geode::client::createAppDomainContext = &Apache::Geode::Client::createAppDomainContext;
--- End diff --
No, it just looked better here since the surrounding code was doing things with the appDomainEnabled flag. Where it came from had no other AppDomain code and didn't check the flag so it was always "enabled". Now it will be enable only when the `appdomain-enabled` property is set.
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106924253
--- Diff: src/cppcache/integration-test/testThinClientExecuteFunctionPrSHOP.cpp ---
@@ -563,7 +563,7 @@ DUNIT_TASK_DEFINITION(CLIENT1, Client1OpTest2)
LOGINFO("FETimeOut begin onRegion");
ExecutionPtr RexecutionPtr = FunctionService::onRegion(regPtr0);
CacheableVectorPtr fe =
- RexecutionPtr->withArgs(CacheableInt32::create(5000))
+ RexecutionPtr->withArgs(CacheableInt32::create(5000 * 1000))
--- End diff --
Should we be marking these areas for posterity and conversion to only using milli?
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106926117
--- Diff: src/cppcache/integration-test/testThinClientPoolExecuteFunction.cpp ---
@@ -1506,48 +1353,10 @@ void runFunctionExecution(bool isEndpoint) {
CALL_TASK(StopC1);
CALL_TASK(CloseServers);
CALL_TASK(CloseLocator1);
-
--- End diff --
What were these big (removed) chunks testing?
- out of scope?
- redundant?
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/geode-native/pull/63
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106928287
--- Diff: src/clicache/src/CacheFactory.cpp ---
@@ -84,6 +85,12 @@ namespace Apache
//TODO::split
SafeConvertClassGeneric::SetAppDomainEnabled(appDomainEnable);
+ if (appDomainEnable)
+ {
+ // Register managed AppDomain context with unmanaged.
+ apache::geode::client::createAppDomainContext = &Apache::Geode::Client::createAppDomainContext;
--- End diff --
I think we may want to just abandon the `appdomain-enabled` property and always enable AppDomain support when using the .NET client. I don't know that there is any good reason for running disabled since .NET uses AppDomains heavily (prior to .NET Core).
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106929690
--- Diff: src/clicache/integration-test/ThinClientAppDomainFunctionExecutionTests.cs ---
@@ -0,0 +1,282 @@
+//=========================================================================
+// Copyright (c) 2002-2014 Pivotal Software, Inc. All Rights Reserved.
+// This product is protected by U.S. and international copyright
+// and intellectual property laws. Pivotal products are covered by
+// more patents listed at http://www.pivotal.io/patents.
+//========================================================================
+
+using System;
+using System.Collections.Generic;
+using System.Collections;
+using System.Threading;
+
+namespace Apache.Geode.Client.UnitTests
+{
+ using NUnit.Framework;
+ using Apache.Geode.DUnitFramework;
+ using Apache.Geode.Client.Tests;
+
+ using Apache.Geode.Client;
+ using Region = Apache.Geode.Client.IRegion<Object, Object>;
+
+ public class MyAppDomainResultCollector<TResult> : IResultCollector<TResult>
+ {
+ #region Private members
+ private bool m_resultReady = false;
+ ICollection<TResult> m_results = null;
+ private int m_addResultCount = 0;
+ private int m_getResultCount = 0;
+ private int m_endResultCount = 0;
+ #endregion
+ public int GetAddResultCount()
+ {
+ return m_addResultCount;
+ }
+ public int GetGetResultCount()
+ {
+ return m_getResultCount;
+ }
+ public int GetEndResultCount()
+ {
+ return m_endResultCount;
+ }
+ public MyAppDomainResultCollector()
+ {
+ m_results = new List<TResult>();
+ }
+ public void AddResult(TResult result)
+ {
+ Util.Log("hitesh in MyAppDomainResultCollector " + result + " : " + result.GetType());
--- End diff --
Doh! thanks!
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106933661
--- Diff: src/clicache/integration-test/ThinClientFunctionExecutionTestsN.cs ---
@@ -959,7 +959,7 @@ public void ExecuteFETimeOut()
region[i] = "VALUE--" + i;
}
- object args = 5000;
+ object args = 5000 * 1000;
--- End diff --
fair choice
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106927339
--- Diff: src/cppcache/integration-test/testThinClientExecuteFunctionPrSHOP.cpp ---
@@ -563,7 +563,7 @@ DUNIT_TASK_DEFINITION(CLIENT1, Client1OpTest2)
LOGINFO("FETimeOut begin onRegion");
ExecutionPtr RexecutionPtr = FunctionService::onRegion(regPtr0);
CacheableVectorPtr fe =
- RexecutionPtr->withArgs(CacheableInt32::create(5000))
+ RexecutionPtr->withArgs(CacheableInt32::create(5000 * 1000))
--- End diff --
Yes. Ultimately we should consider removing the continuity issue between the client API and the server. The client is taking the timeout as seconds but sending to the server as milliseconds. Ultimately taking a chrono::duration would make the API more clear. The test would still need to be very explicit on the use of milliseconds are the argument to the function.
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106926886
--- Diff: src/clicache/integration-test/ThinClientFunctionExecutionTestsN.cs ---
@@ -959,7 +959,7 @@ public void ExecuteFETimeOut()
region[i] = "VALUE--" + i;
}
- object args = 5000;
+ object args = 5000 * 1000;
--- End diff --
maybe a constant..
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106924902
--- Diff: src/cppcache/integration-test/testThinClientPoolExecuteFunction.cpp ---
@@ -1158,9 +1160,10 @@ DUNIT_TASK_DEFINITION(CLIENT1, Client2OpTest)
LOGINFO("FETimeOut begin onServer");
ExecutionPtr serverExc = FunctionService::onServer(getHelper()->cachePtr);
- CacheableVectorPtr vec = serverExc->withArgs(CacheableInt32::create(5000))
- ->execute(FETimeOut, 5000)
- ->getResult();
+ CacheableVectorPtr vec =
+ serverExc->withArgs(CacheableInt32::create(5000 * 1000))
--- End diff --
might consider making a constant or two for these literals... in case someone is maintaining the test later on and needs/wants to adjust times used
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106930234
--- Diff: src/cppcache/integration-test/testThinClientPoolExecuteFunction.cpp ---
@@ -1158,9 +1160,10 @@ DUNIT_TASK_DEFINITION(CLIENT1, Client2OpTest)
LOGINFO("FETimeOut begin onServer");
ExecutionPtr serverExc = FunctionService::onServer(getHelper()->cachePtr);
- CacheableVectorPtr vec = serverExc->withArgs(CacheableInt32::create(5000))
- ->execute(FETimeOut, 5000)
- ->getResult();
+ CacheableVectorPtr vec =
+ serverExc->withArgs(CacheableInt32::create(5000 * 1000))
--- End diff --
See comment on same request below.
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by PivotalSarge <gi...@git.apache.org>.
Github user PivotalSarge commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106921830
--- Diff: src/cppcache/integration-test/testThinClientExecuteFunctionPrSHOP.cpp ---
@@ -563,7 +563,7 @@ DUNIT_TASK_DEFINITION(CLIENT1, Client1OpTest2)
LOGINFO("FETimeOut begin onRegion");
ExecutionPtr RexecutionPtr = FunctionService::onRegion(regPtr0);
CacheableVectorPtr fe =
- RexecutionPtr->withArgs(CacheableInt32::create(5000))
+ RexecutionPtr->withArgs(CacheableInt32::create(5000 * 1000))
--- End diff --
Would std::chrono::duration help avoid the seconds-for-milliseconds kinds of problems in future?
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by echobravopapa <gi...@git.apache.org>.
Github user echobravopapa commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106929938
--- Diff: src/clicache/src/CacheFactory.cpp ---
@@ -84,6 +85,12 @@ namespace Apache
//TODO::split
SafeConvertClassGeneric::SetAppDomainEnabled(appDomainEnable);
+ if (appDomainEnable)
+ {
+ // Register managed AppDomain context with unmanaged.
+ apache::geode::client::createAppDomainContext = &Apache::Geode::Client::createAppDomainContext;
--- End diff --
+1 make it a built default, less property juggling for the app devs
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by mmartell <gi...@git.apache.org>.
Github user mmartell commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106929025
--- Diff: src/clicache/integration-test/ThinClientAppDomainFunctionExecutionTests.cs ---
@@ -0,0 +1,282 @@
+//=========================================================================
+// Copyright (c) 2002-2014 Pivotal Software, Inc. All Rights Reserved.
+// This product is protected by U.S. and international copyright
+// and intellectual property laws. Pivotal products are covered by
+// more patents listed at http://www.pivotal.io/patents.
+//========================================================================
+
+using System;
+using System.Collections.Generic;
+using System.Collections;
+using System.Threading;
+
+namespace Apache.Geode.Client.UnitTests
+{
+ using NUnit.Framework;
+ using Apache.Geode.DUnitFramework;
+ using Apache.Geode.Client.Tests;
+
+ using Apache.Geode.Client;
+ using Region = Apache.Geode.Client.IRegion<Object, Object>;
+
+ public class MyAppDomainResultCollector<TResult> : IResultCollector<TResult>
+ {
+ #region Private members
+ private bool m_resultReady = false;
+ ICollection<TResult> m_results = null;
+ private int m_addResultCount = 0;
+ private int m_getResultCount = 0;
+ private int m_endResultCount = 0;
+ #endregion
+ public int GetAddResultCount()
+ {
+ return m_addResultCount;
+ }
+ public int GetGetResultCount()
+ {
+ return m_getResultCount;
+ }
+ public int GetEndResultCount()
+ {
+ return m_endResultCount;
+ }
+ public MyAppDomainResultCollector()
+ {
+ m_results = new List<TResult>();
+ }
+ public void AddResult(TResult result)
+ {
+ Util.Log("hitesh in MyAppDomainResultCollector " + result + " : " + result.GetType());
--- End diff --
What's the significance of "hitesh in"?
---
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] geode-native pull request #63: GEODE-2657 - Fixes and tests Function Executi...
Posted by pivotal-jbarrett <gi...@git.apache.org>.
Github user pivotal-jbarrett commented on a diff in the pull request:
https://github.com/apache/geode-native/pull/63#discussion_r106929867
--- Diff: src/clicache/integration-test/ThinClientFunctionExecutionTestsN.cs ---
@@ -959,7 +959,7 @@ public void ExecuteFETimeOut()
region[i] = "VALUE--" + i;
}
- object args = 5000;
+ object args = 5000 * 1000;
--- End diff --
I felt it was important to call out the disconnect between units here rather than hide it in a constant.
---
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.
---