timoninmaxim commented on a change in pull request #9807:
URL: https://github.com/apache/ignite/pull/9807#discussion_r811028205
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/consistency/GridNearReadRepairAbstractFuture.java
##########
@@ -58,15 +57,12 @@
/** Maximum number of attempts to remap key to the same primary node. */
protected static final int MAX_REMAP_CNT =
getInteger(IGNITE_NEAR_GET_MAX_REMAPS, DFLT_MAX_REMAP_CNT);
- /** Remap count updater. */
- protected static final
AtomicIntegerFieldUpdater<GridNearReadRepairAbstractFuture> REMAP_CNT_UPD =
-
AtomicIntegerFieldUpdater.newUpdater(GridNearReadRepairAbstractFuture.class,
"remapCnt");
-
- /** Remap count. */
- protected volatile int remapCnt;
+ /** Lsnr calls upd. */
+ private static final
AtomicIntegerFieldUpdater<GridNearReadRepairAbstractFuture> LSNR_CALLS_UPD =
Review comment:
Looks like we can use AtomicInteger here without price. This future is
created for whole batch of keys, then no many of instances of the future exist
(actually only single one), doesn't it?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/GridNearTxLocal.java
##########
@@ -2455,6 +2455,11 @@ private boolean enlistWriteEntry(GridCacheContext
cacheCtx,
}
if (readRepairStrategy != null) { // Checking and
repairing each locked entry (if necessary).
+ AffinityTopologyVersion topVer =
topologyVersionSnapshot();
+
+ if (topVer == null)
+ topVer = entryTopVer;
Review comment:
There is no docs for `entryTopVer` in the method signature. Why do you
check it after `topologyVersionSnapshot()`? What is a full priority queue of
topology versions?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/consistency/GridNearReadRepairAbstractFuture.java
##########
@@ -261,10 +257,19 @@ else if (!canRemap)
return;
}
+ else
+ LSNR_CALLS_UPD.incrementAndGet(this);
Review comment:
It's possible to use returned value here in next lines.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/consistency/GridNearReadRepairAbstractFuture.java
##########
@@ -261,10 +257,19 @@ else if (!canRemap)
return;
}
+ else
+ LSNR_CALLS_UPD.incrementAndGet(this);
- for (GridPartitionedGetFuture<KeyCacheObject, EntryGetResult> fut :
futs.values()) {
- if (!fut.isDone() || fut.error() != null)
+ assert lsnrCalls <= futs.size();
+
+ if (isDone() || lsnrCalls != futs.size())
+ return;
+
+ synchronized (this) {
Review comment:
I think it's possible to use `lsnrCalls` value immediately after
incrementing it. This value is guarded from concurrency because incremented
atomically, so we can leverage on this and remove any sync here. Am I missing
smth here?
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/consistency/GridNearReadRepairCheckOnlyFuture.java
##########
@@ -125,6 +146,8 @@ public GridNearReadRepairCheckOnlyFuture(
* @return Future represents 1 entry's value.
*/
public <K, V> IgniteInternalFuture<V> single() {
+ init();
Review comment:
What if we add a parameter with an initialization flag for the
constructor `GridNearReadRepairAbstractFuture`? Now it looks strange and a
little bit dangerous (it's possible to forget to `init()` future somewhere)
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/consistency/GridNearReadRepairAbstractFuture.java
##########
@@ -93,16 +89,25 @@
protected final IgniteInternalTx tx;
/** Primaries per key. */
- protected volatile Map<KeyCacheObject, ClusterNode> primaries;
+ protected final Map<KeyCacheObject, ClusterNode> primaries = new
HashMap<>();
/** Strategy. */
protected final ReadRepairStrategy strategy;
+ /** Remap count. */
+ protected volatile int remapCnt;
Review comment:
Looks like this definition wasn't changed but just moved lower (from
line 66). Let's keep it on previous place
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/consistency/GridNearReadRepairAbstractFuture.java
##########
@@ -93,16 +89,25 @@
protected final IgniteInternalTx tx;
/** Primaries per key. */
- protected volatile Map<KeyCacheObject, ClusterNode> primaries;
+ protected final Map<KeyCacheObject, ClusterNode> primaries = new
HashMap<>();
Review comment:
Let's define it in constructor as `Collections.unmodifiableMap()` of
local HashMap.
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/near/consistency/GridNearReadRepairAbstractFuture.java
##########
@@ -58,15 +57,12 @@
/** Maximum number of attempts to remap key to the same primary node. */
protected static final int MAX_REMAP_CNT =
getInteger(IGNITE_NEAR_GET_MAX_REMAPS, DFLT_MAX_REMAP_CNT);
- /** Remap count updater. */
- protected static final
AtomicIntegerFieldUpdater<GridNearReadRepairAbstractFuture> REMAP_CNT_UPD =
-
AtomicIntegerFieldUpdater.newUpdater(GridNearReadRepairAbstractFuture.class,
"remapCnt");
-
- /** Remap count. */
- protected volatile int remapCnt;
+ /** Lsnr calls upd. */
+ private static final
AtomicIntegerFieldUpdater<GridNearReadRepairAbstractFuture> LSNR_CALLS_UPD =
+
AtomicIntegerFieldUpdater.newUpdater(GridNearReadRepairAbstractFuture.class,
"lsnrCalls");
/** Affinity node's get futures. */
- protected final Map<ClusterNode, GridPartitionedGetFuture<KeyCacheObject,
EntryGetResult>> futs = new ConcurrentHashMap<>();
+ protected final Map<ClusterNode, GridPartitionedGetFuture<KeyCacheObject,
EntryGetResult>> futs = new HashMap<>();
Review comment:
Let's define it in constructor as `Collections.unmodifiableMap()` of
local HashMap.
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
