[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/geode/pull/589


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/589#discussion_r123845237
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
 ---
@@ -37,20 +38,25 @@
 
   private String functionId = null;
 
+  private Cache cache = null;
+
   private ResultSender resultSender = null;
 
   private final boolean isPossDup;
 
   public FunctionContextImpl(final String functionId, final Object args,
   ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+  }
+
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
   }
 
-  public FunctionContextImpl(final String functionId, final Object args, 
ResultSender resultSender,
-  boolean isPossibleDuplicate) {
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --

I agree, once we get rid of that static gemfire cache function, these 
changes will be necessary. better start making the changes somewhere. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext

[jhuynh1]

Github user jhuynh1 commented on a diff in the pull request:

https://github.com/apache/geode/pull/589#discussion_r123830138
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
 ---
@@ -37,20 +38,25 @@
 
   private String functionId = null;
 
+  private Cache cache = null;
+
   private ResultSender resultSender = null;
 
   private final boolean isPossDup;
 
   public FunctionContextImpl(final String functionId, final Object args,
   ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+  }
+
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
   }
 
-  public FunctionContextImpl(final String functionId, final Object args, 
ResultSender resultSender,
-  boolean isPossibleDuplicate) {
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --

Sure we can't remove GemFireCacheImpl.getInstance now and we might not be 
able to ever... but in order for it to be possible, this change would need to 
be made anyways if I am not mistaken... not making these constructor changes 
will hinder the effort later.  Since the work is already done, was there a 
reason why we didn't want to make these signature changes?  Is it too 
cumbersome with the number of variables being passed 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/589#discussion_r123818259
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
 ---
@@ -37,20 +38,25 @@
 
   private String functionId = null;
 
+  private Cache cache = null;
+
   private ResultSender resultSender = null;
 
   private final boolean isPossDup;
 
   public FunctionContextImpl(final String functionId, final Object args,
   ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+  }
+
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
   }
 
-  public FunctionContextImpl(final String functionId, final Object args, 
ResultSender resultSender,
-  boolean isPossibleDuplicate) {
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --

we can't remove GemFireCacheImpl.getInstance() for now, that's the way we 
are getting the instance to create the FunctionContext in the first place, why 
not just use it to avoid making more constructor changes. This just my 2 cents. 
I can go either way on this though.

The problem this changeset is trying to solve is that if external developer 
can not use GemfireCacheImpl.getInstance() to get the cache since it's an 
internal api, they can only use CacheFactory.getAnyIntance() and that's a 
synchronized static, so it requires a lock on CachFactory which creates some 
deadlocks.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext

[jhuynh1]

Github user jhuynh1 commented on a diff in the pull request:

https://github.com/apache/geode/pull/589#discussion_r123553740
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
 ---
@@ -37,20 +38,25 @@
 
   private String functionId = null;
 
+  private Cache cache = null;
+
   private ResultSender resultSender = null;
 
   private final boolean isPossDup;
 
   public FunctionContextImpl(final String functionId, final Object args,
   ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+  }
+
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
   }
 
-  public FunctionContextImpl(final String functionId, final Object args, 
ResultSender resultSender,
-  boolean isPossibleDuplicate) {
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --

I think the goal is to remove static calls altogether at some point.  I 
think some benefits would be it makes it easier to write test code for 
functions (for customers) and our own functions.  Client code could already use 
GemFireCacheImpl.getInstance() but I think the goal was to remove this call one 
day in the far 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/589#discussion_r123544960
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
 ---
@@ -37,20 +38,25 @@
 
   private String functionId = null;
 
+  private Cache cache = null;
+
   private ResultSender resultSender = null;
 
   private final boolean isPossDup;
 
   public FunctionContextImpl(final String functionId, final Object args,
   ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+  }
+
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
   }
 
-  public FunctionContextImpl(final String functionId, final Object args, 
ResultSender resultSender,
-  boolean isPossibleDuplicate) {
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --

or better yet, use GemfireCacheImpl.getExisting() instead of getInstance 
because the first one provide checking for existing cache.


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/589#discussion_r123544587
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
 ---
@@ -37,20 +38,25 @@
 
   private String functionId = null;
 
+  private Cache cache = null;
+
   private ResultSender resultSender = null;
 
   private final boolean isPossDup;
 
   public FunctionContextImpl(final String functionId, final Object args,
   ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+  }
+
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
   }
 
-  public FunctionContextImpl(final String functionId, final Object args, 
ResultSender resultSender,
-  boolean isPossibleDuplicate) {
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --

Looking at the places where it's grabbing the cache to create the 
FunctionContext, it's all using GemfireCacheImpl.getInstance(). I am wondering 
we should just have the implementation of getCache() in FunctionContextImpl to 
be simply GemfireCacheImpl.getInstance(), then we don't need to change all the 
constructor signatures and keep a member variable of the cache in the context. 


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext

[jinmeiliao]

Github user jinmeiliao commented on a diff in the pull request:

https://github.com/apache/geode/pull/589#discussion_r123332996
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/internal/cache/execute/FunctionContextImpl.java
 ---
@@ -37,20 +38,25 @@
 
   private String functionId = null;
 
+  private Cache cache = null;
+
   private ResultSender resultSender = null;
 
   private final boolean isPossDup;
 
   public FunctionContextImpl(final String functionId, final Object args,
   ResultSender resultSender) {
-this.functionId = functionId;
-this.args = args;
-this.resultSender = resultSender;
-this.isPossDup = false;
+this(null, functionId, args, resultSender, false);
+  }
+
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender) {
+this(cache, functionId, args, resultSender, false);
   }
 
-  public FunctionContextImpl(final String functionId, final Object args, 
ResultSender resultSender,
-  boolean isPossibleDuplicate) {
+  public FunctionContextImpl(final Cache cache, final String functionId, 
final Object args,
+  ResultSender resultSender, boolean isPossibleDuplicate) {
--- End diff --

this could still have have cache end up being null if user did not create 
the FunctionContextImpl with a cache, either pass in null, or use the other 
non-cache constructor. It would be good to add a check here:
if(cache==null){
  cache = GemfireCacheImpl.getExisting();
}


---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #589: GEODE-393: Providing cache for FunctionContext

[DivineEnder]

GitHub user DivineEnder opened a pull request:

https://github.com/apache/geode/pull/589

GEODE-393: Providing cache for FunctionContext

Added the ability to pass a Cache to a function context as well as tests to 
check for this.

Thank you for submitting a contribution to Apache Geode.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in 
the commit message?

- [ ] Has your PR been rebased against the latest commit within the target 
branch (typically `develop`)?

- [ ] Is your initial contribution a single, squashed commit?

- [ ] Does `gradlew build` run cleanly?

- [ ] Have you written or updated unit tests to verify your changes?

- [ ] If adding new dependencies to the code, are these dependencies 
licensed in a way that is compatible for inclusion under [ASF 
2.0](http://www.apache.org/legal/resolved.html#category-a)?

### Note:
Please ensure that once the PR is submitted, you check travis-ci for build 
issues and
submit an update to your PR as soon as possible. If you need help, please 
send an
email to dev@geode.apache.org.


You can merge this pull request into a Git repository by running:

$ git pull https://github.com/DivineEnder/geode feature/GEODE-393

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/geode/pull/589.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 #589


commit ec1047c56ebc9f92e7b521bd765286fbed09af59
Author: David Anuta 
Date:   2017-06-16T23:05:24Z

GEODE-393: Providing cache for FunctionContext

Added the ability to pass a Cache to a function context as well as tests to 
check for 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---