You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/02/21 13:23:42 UTC
[GitHub] [ignite] timoninmaxim commented on a change in pull request #9807: IGNITE-16071 Read Repair futures should be rewritten to use wait-free sync instead of synchronized
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org