issue-3097 : re-writting getAll() impl by nishchay-naresh · Pull Request #3130 · ehcache/ehcache3

nishchay-naresh · 2023-02-23T11:05:50Z

No description provided.

nishchay-naresh · 2023-02-23T11:08:46Z

Proposed changes are in early stage. Opening the PR to have effective feedback
PS - Not utilizing the mappingFunction while fetching the mappings. @Ignoring all the test who all are mappingFunction based fetch

chrisdennis

Subject: [PATCH] Code Review Comments
---
Index: ehcache-core/src/main/java/org/ehcache/core/spi/store/tiering/AuthoritativeTier.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ehcache-core/src/main/java/org/ehcache/core/spi/store/tiering/AuthoritativeTier.java b/ehcache-core/src/main/java/org/ehcache/core/spi/store/tiering/AuthoritativeTier.java
--- a/ehcache-core/src/main/java/org/ehcache/core/spi/store/tiering/AuthoritativeTier.java	(revision 154708ade16df25847e8b9fbf4a583fba27d5c06)
+++ b/ehcache-core/src/main/java/org/ehcache/core/spi/store/tiering/AuthoritativeTier.java	(date 1681135893484)
@@ -76,8 +76,7 @@
    */
   void setInvalidationValve(InvalidationValve valve);
 
-
-  Iterable<? extends Map.Entry<? extends K,? extends V>> bulkComputeIfAbsentAndFault(Iterable<? extends K> keyParam, Function<Iterable<? extends K>, Iterable<? extends Map.Entry<? extends K,? extends V>>> mappingFunction) throws StoreAccessException;
+  Iterable<? extends Map.Entry<? extends K,? extends ValueHolder<V>>> bulkComputeIfAbsentAndFault(Iterable<? extends K> keyParam, Function<Iterable<? extends K>, Iterable<? extends Map.Entry<? extends K,? extends V>>> mappingFunction) throws StoreAccessException;
 
     /**
    * Invalidation valve, that is the mechanism through which an {@link AuthoritativeTier} can request invalidations
Index: ehcache-impl/src/main/java/org/ehcache/impl/internal/store/basic/NopStore.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/basic/NopStore.java b/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/basic/NopStore.java
--- a/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/basic/NopStore.java	(revision 154708ade16df25847e8b9fbf4a583fba27d5c06)
+++ b/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/basic/NopStore.java	(date 1681135486810)
@@ -68,7 +68,7 @@
   }
 
   @Override
-  public Iterable<? extends Map.Entry<? extends K, ? extends V>> bulkComputeIfAbsentAndFault(Iterable<? extends K> keyParam, Function<Iterable<? extends K>, Iterable<? extends Map.Entry<? extends K, ? extends V>>> mappingFunction) throws StoreAccessException {
+  public Iterable<? extends Map.Entry<? extends K, ? extends ValueHolder<V>>> bulkComputeIfAbsentAndFault(Iterable<? extends K> keyParam, Function<Iterable<? extends K>, Iterable<? extends Map.Entry<? extends K, ? extends V>>> mappingFunction) throws StoreAccessException {
     return null;
   }
 
Index: ehcache-impl/src/main/java/org/ehcache/impl/internal/store/tiering/TieredStore.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/tiering/TieredStore.java b/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/tiering/TieredStore.java
--- a/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/tiering/TieredStore.java	(revision 154708ade16df25847e8b9fbf4a583fba27d5c06)
+++ b/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/tiering/TieredStore.java	(date 1681135391217)
@@ -361,7 +361,7 @@
     try {
       return cachingTier().bulkGetOrComputeIfAbsent(keys, missingKeys -> {
         try {
-          return authoritativeTier.bulkComputeIfAbsent(missingKeys, mappingFunction).entrySet();
+          return authoritativeTier.bulkComputeIfAbsentAndFault(missingKeys, mappingFunction);
         } catch (StoreAccessException cae) {
           throw new StorePassThroughException(cae);
         }
Index: clustered/ehcache-client/src/main/java/org/ehcache/clustered/client/internal/store/ClusteredStore.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/clustered/ehcache-client/src/main/java/org/ehcache/clustered/client/internal/store/ClusteredStore.java b/clustered/ehcache-client/src/main/java/org/ehcache/clustered/client/internal/store/ClusteredStore.java
--- a/clustered/ehcache-client/src/main/java/org/ehcache/clustered/client/internal/store/ClusteredStore.java	(revision 154708ade16df25847e8b9fbf4a583fba27d5c06)
+++ b/clustered/ehcache-client/src/main/java/org/ehcache/clustered/client/internal/store/ClusteredStore.java	(date 1681135472355)
@@ -596,10 +596,10 @@
   }
 
   @Override
-  public Iterable<? extends Map.Entry<? extends K, ? extends V>> bulkComputeIfAbsentAndFault(Iterable<? extends K> keys, Function<Iterable<? extends K>, Iterable<? extends Map.Entry<? extends K, ? extends V>>> mappingFunction) throws StoreAccessException {
-    Map<K, V> result = new HashMap<>();
+  public Iterable<? extends Map.Entry<? extends K, ? extends ValueHolder<V>>> bulkComputeIfAbsentAndFault(Iterable<? extends K> keys, Function<Iterable<? extends K>, Iterable<? extends Map.Entry<? extends K, ? extends V>>> mappingFunction) throws StoreAccessException {
+    Map<K, ValueHolder<V>> result = new HashMap<>();
     for (K key : keys) {
-      result.put(key, getAndFault(key).get());
+      result.put(key, getAndFault(key));
     }
     return result.entrySet();
   }
Index: ehcache-impl/src/main/java/org/ehcache/impl/internal/store/offheap/AbstractOffHeapStore.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/offheap/AbstractOffHeapStore.java b/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/offheap/AbstractOffHeapStore.java
--- a/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/offheap/AbstractOffHeapStore.java	(revision 154708ade16df25847e8b9fbf4a583fba27d5c06)
+++ b/ehcache-impl/src/main/java/org/ehcache/impl/internal/store/offheap/AbstractOffHeapStore.java	(date 1681135526749)
@@ -809,7 +809,7 @@
           return null;
         }
       };
-      ValueHolder<V> computed = computeIfAbsentAndFault(key, function);
+      ValueHolder<V> computed = computeIfAbsent(key, function);
       result.put(key, computed);
     }
     return result;
@@ -846,8 +846,8 @@
     return mappedValue;
   }
 
-  public Iterable<? extends Map.Entry<? extends K, ? extends V>> bulkComputeIfAbsentAndFault(Iterable<? extends K> keys, Function<Iterable<? extends K>, Iterable<? extends Map.Entry<? extends K, ? extends V>>> mappingFunction) throws StoreAccessException {
-    Map<K, V> result = new HashMap<>();
+  public Iterable<? extends Map.Entry<? extends K, ? extends ValueHolder<V>>> bulkComputeIfAbsentAndFault(Iterable<? extends K> keys, Function<Iterable<? extends K>, Iterable<? extends Map.Entry<? extends K, ? extends V>>> mappingFunction) throws StoreAccessException {
+    Map<K, ValueHolder<V>> result = new HashMap<>();
     for (K key : keys) {
       checkKey(key);
       Function<K, V> function = k -> {
@@ -860,8 +860,7 @@
           return null;
         }
       };
-      ValueHolder<V> computed = computeIfAbsent(key, function);
-      result.put(key, computed.get());
+      result.put(key, computeIfAbsentAndFault(key, function));
     }
     return result.entrySet();
   }

chrisdennis

Test changes look like they're moving in the right direction. Lets chat tomorrow about how the tests look now.

...ehcache-client/src/main/java/org/ehcache/clustered/client/internal/store/ClusteredStore.java

core-spi-test/src/main/java/org/ehcache/internal/store/StoreSPITest.java

...ehcache-client/src/main/java/org/ehcache/clustered/client/internal/store/ClusteredStore.java

ehcache-core/src/main/java/org/ehcache/core/spi/store/tiering/AuthoritativeTier.java

ehcache-core/src/main/java/org/ehcache/core/spi/store/tiering/CachingTier.java

ehcache-impl/src/main/java/org/ehcache/impl/internal/store/basic/NopStore.java

ehcache-impl/src/main/java/org/ehcache/impl/internal/store/tiering/TieredStore.java

ehcache-impl/src/test/java/org/ehcache/impl/internal/store/heap/OnHeapStoreBulkMethodsTest.java

chrisdennis

Approved... with one minor question. Probably not a big deal... but would be good to know which it should be.

ehcache-impl/src/main/java/org/ehcache/impl/internal/store/tiering/TieredStore.java

chrisdennis · 2023-08-16T19:56:17Z

Probably makes sense to rebase this and do some squashing.

ehcache-core/src/main/java/org/ehcache/core/spi/store/tiering/AuthoritativeTier.java

ehcache-core/src/main/java/org/ehcache/core/spi/store/tiering/CachingTier.java

nishchay-naresh requested review from AbfrmBlr and chrisdennis February 23, 2023 11:05

chrisdennis requested changes Apr 10, 2023

View reviewed changes

nishchay-naresh requested a review from chrisdennis May 8, 2023 13:09

chrisdennis reviewed May 8, 2023

View reviewed changes

chrisdennis self-requested a review May 18, 2023 21:08

chrisdennis requested changes May 18, 2023

View reviewed changes

...ehcache-client/src/main/java/org/ehcache/clustered/client/internal/store/ClusteredStore.java Show resolved Hide resolved

nishchay-naresh requested review from chrisdennis, cljohnso, jhouserizer and prasantakumar82 June 20, 2023 14:51

chrisdennis requested changes Jul 18, 2023

View reviewed changes

chrisdennis approved these changes Aug 16, 2023

View reviewed changes

ehcache-impl/src/main/java/org/ehcache/impl/internal/store/tiering/TieredStore.java Outdated Show resolved Hide resolved

chrisdennis requested changes Aug 17, 2023

View reviewed changes

issue-3097 : re-writting getAll() implementation

c4ca535

nishchay-naresh force-pushed the issue-3097 branch from 6b2129a to c4ca535 Compare August 18, 2023 12:51

chrisdennis self-requested a review August 18, 2023 13:42

chrisdennis approved these changes Aug 18, 2023

View reviewed changes

chrisdennis merged commit 5f43856 into ehcache:master Aug 18, 2023

nishchay-naresh mentioned this pull request Aug 18, 2023

TieredStore doesn't populate the caching tier on getAll(...) #3097

Closed

Conversation

nishchay-naresh commented Feb 23, 2023

Uh oh!

nishchay-naresh commented Feb 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrisdennis left a comment

Choose a reason for hiding this comment

Uh oh!

chrisdennis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chrisdennis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisdennis commented Aug 16, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nishchay-naresh commented Feb 23, 2023 •

edited

Loading