[GitHub] [geode] DonalEvans commented on a change in pull request #7295: GEODE-9922: Move Redis cross-slot checking to RegionProvider

[GitBox <gi...@apache.org>]

DonalEvans commented on a change in pull request #7295:
URL: https://github.com/apache/geode/pull/7295#discussion_r789903022



##########
File path: geode-for-redis/src/main/java/org/apache/geode/redis/internal/services/RegionProvider.java
##########
@@ -155,6 +160,18 @@ public String getRedisRegionName() {
     }
   }
 
+  @VisibleForTesting
+  static boolean areKeysCrossSlots(List<RedisKey> keysToLock) {
+    int slot = keysToLock.get(0).getSlot();
+    for (int i = 1, keysToLockSize = keysToLock.size(); i < keysToLockSize; i++) {

Review comment:
       You're correct. I was originally iterating the whole list using a foreach loop, but since there wasn't any reason to compare the first key's slot to itself, I used IntelliJ's automatic refactor to turn it into an "optimized indexed for loop" which is what this is. I think the reason it's formatted like that with `keysToLockSize` defined in the for expression is so that we only call `keysToLock.size()` once to initialize it, rather than calling it every time the loop executes.
   
   Given the cost of comparing the first key's slot to itself is negligible, I might just put it back to how I originally had it, since it makes the code a bit clearer.




-- 
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.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

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