You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/05/16 02:02:42 UTC

[GitHub] [geode] dschneider-pivotal opened a new pull request #5125: Change more hash commands to use function+delta

dschneider-pivotal opened a new pull request #5125:
URL: https://github.com/apache/geode/pull/5125


   In addition to the hash commands this branch also changes smove to use functions.
   
   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, check Concourse 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.
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] dschneider-pivotal merged pull request #5125: GEODE-8170: Change all hash and set commands to use function+delta

Posted by GitBox <gi...@apache.org>.
dschneider-pivotal merged pull request #5125:
URL: https://github.com/apache/geode/pull/5125


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5125: Change more hash commands to use function+delta

Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5125:
URL: https://github.com/apache/geode/pull/5125#discussion_r426723568



##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/CommandFunction.java
##########
@@ -117,6 +118,32 @@ protected Object compute(Region localRegion, ByteArrayWrapper key,
             () -> new RedisSetInRegion(localRegion).sscan(key, matchPattern, count, cursor);
         break;
       }
+      case SCREATE: {

Review comment:
       I think the place I was using it was refactored and now it is not used. I'll remove it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5125: Change more hash commands to use function+delta

Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5125:
URL: https://github.com/apache/geode/pull/5125#discussion_r426703666



##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/CommandFunction.java
##########
@@ -117,6 +118,32 @@ protected Object compute(Region localRegion, ByteArrayWrapper key,
             () -> new RedisSetInRegion(localRegion).sscan(key, matchPattern, count, cursor);
         break;
       }
+      case SCREATE: {

Review comment:
       How is this used / invoked?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5125: Change more hash commands to use function+delta

Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5125:
URL: https://github.com/apache/geode/pull/5125#discussion_r426723142



##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/EmptyRedisHash.java
##########
@@ -26,41 +25,26 @@
 import org.apache.geode.redis.internal.executor.hash.RedisHash;
 
 public class EmptyRedisHash extends RedisHash {
-  @Override
-  public synchronized int hset(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
-      List<ByteArrayWrapper> fieldsToSet, boolean nx) {
-    throw new UnsupportedOperationException();
-  }
-
-  @Override
-  public synchronized int hdel(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
-      List<ByteArrayWrapper> fieldsToRemove) {
-    return 0;
-  }
-
-  @Override
-  public synchronized Collection<ByteArrayWrapper> hgetall() {
-    return emptyList();
+  public EmptyRedisHash() {

Review comment:
       We do have to be careful that our canonical instance is immutable




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5125: Change more hash commands to use function+delta

Posted by GitBox <gi...@apache.org>.
dschneider-pivotal commented on a change in pull request #5125:
URL: https://github.com/apache/geode/pull/5125#discussion_r426722931



##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/EmptyRedisHash.java
##########
@@ -26,41 +25,26 @@
 import org.apache.geode.redis.internal.executor.hash.RedisHash;
 
 public class EmptyRedisHash extends RedisHash {
-  @Override
-  public synchronized int hset(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
-      List<ByteArrayWrapper> fieldsToSet, boolean nx) {
-    throw new UnsupportedOperationException();
-  }
-
-  @Override
-  public synchronized int hdel(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
-      List<ByteArrayWrapper> fieldsToRemove) {
-    return 0;
-  }
-
-  @Override
-  public synchronized Collection<ByteArrayWrapper> hgetall() {
-    return emptyList();
+  public EmptyRedisHash() {

Review comment:
       It is just an optimization. It gives us a single instance of RedisHash that represents an empty hash instead of having to create a new instance every time we want to represent an empty hash. We do this alot because we have a common pattern of not wanting to return null when we do the initial lookup of a RedisHash from the region.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode] jdeppe-pivotal commented on a change in pull request #5125: Change more hash commands to use function+delta

Posted by GitBox <gi...@apache.org>.
jdeppe-pivotal commented on a change in pull request #5125:
URL: https://github.com/apache/geode/pull/5125#discussion_r426702847



##########
File path: geode-redis/src/main/java/org/apache/geode/redis/internal/executor/EmptyRedisHash.java
##########
@@ -26,41 +25,26 @@
 import org.apache.geode.redis.internal.executor.hash.RedisHash;
 
 public class EmptyRedisHash extends RedisHash {
-  @Override
-  public synchronized int hset(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
-      List<ByteArrayWrapper> fieldsToSet, boolean nx) {
-    throw new UnsupportedOperationException();
-  }
-
-  @Override
-  public synchronized int hdel(Region<ByteArrayWrapper, RedisData> region, ByteArrayWrapper key,
-      List<ByteArrayWrapper> fieldsToRemove) {
-    return 0;
-  }
-
-  @Override
-  public synchronized Collection<ByteArrayWrapper> hgetall() {
-    return emptyList();
+  public EmptyRedisHash() {

Review comment:
       Do we still need this class? I'm not sure I fully understand it's usage since when I switched `RedisHash.EMPTY` to `new RedisHash(Collections.emptyList())`, `HashesIntegrationTest` still passes.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org