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 2021/02/23 14:46:37 UTC
[GitHub] [ignite] anton-vinogradov opened a new pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
anton-vinogradov opened a new pull request #8822:
URL: https://github.com/apache/ignite/pull/8822
…g some time) after the Cellular switch
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
----------------------------------------------------------------
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] [ignite] alex-plekhanov commented on a change in pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
Posted by GitBox <gi...@apache.org>.
alex-plekhanov commented on a change in pull request #8822:
URL: https://github.com/apache/ignite/pull/8822#discussion_r588105977
##########
File path: modules/core/src/test/java/org/apache/ignite/testframework/junits/common/GridCommonAbstractTest.java
##########
@@ -2419,6 +2422,35 @@ protected void assertCountersSame(int partId, boolean withReserveCntr) throws As
}
}
+ /**
+ * @param partId Partition.
+ * @param withReserveCntr {@code True} to compare reserve counters. Because reserve counters are synced during
+ * @param cacheName Cache name.
+ * @param cnt Counter.
+ * @param reserved Reserved counter.
+ * PME invoking with {@code true} makes sense only after PME was finished.
+ */
+ protected void assertCountersAsExpected(int partId, boolean withReserveCntr, String cacheName, long cnt,
+ long reserved) throws AssertionFailedError {
+ List<T3<String, @Nullable PartitionUpdateCounter, Boolean>> cntrMap = G.allGrids().stream().filter(ignite ->
+ !ignite.configuration().isClientMode()).map(ignite ->
+ new T3<>(ignite.name(), counter(partId, cacheName, ignite.name()),
+ ignite.affinity(cacheName).isPrimary(ignite.cluster().localNode(), partId))).collect(toList());
+
+ for (T3<String, PartitionUpdateCounter, Boolean> cntr : cntrMap) {
+ if (cntr.get2() == null)
+ continue;
+
+ assertEquals("Expecting same counters [partId=" + partId +
+ ", cntrs=" + cntrMap + ']', cnt, cntr.get2().get());
+
+ if (withReserveCntr)
Review comment:
Codestyle: `{}` for multiline `if`
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/preloader/GridDhtPartitionsExchangeFuture.java
##########
@@ -1634,41 +1631,12 @@ private void distributedExchange() throws IgniteCheckedException {
boolean skipWaitOnLocalJoin = localJoinExchange()
&& cctx.exchange().latch().canSkipJoiningNodes(initialVersion());
- if (context().exchangeFreeSwitch() && isBaselineNodeFailed()) {
+ if (context().exchangeFreeSwitch() && isBaselineNodeFailed())
Review comment:
Codestyle: `{}` for multiline `if`
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/preloader/GridDhtPartitionsExchangeFuture.java
##########
@@ -1634,41 +1631,12 @@ private void distributedExchange() throws IgniteCheckedException {
boolean skipWaitOnLocalJoin = localJoinExchange()
&& cctx.exchange().latch().canSkipJoiningNodes(initialVersion());
- if (context().exchangeFreeSwitch() && isBaselineNodeFailed()) {
+ if (context().exchangeFreeSwitch() && isBaselineNodeFailed())
// Currently MVCC does not support operations on partially switched cluster.
if (cctx.kernalContext().coordinators().mvccEnabled())
- waitPartitionRelease(EXCHANGE_FREE_LATCH_ID, true, false, null);
- else {
- boolean partitionedRecoveryRequired = rebalancedInfo.primaryNodes.contains(firstDiscoEvt.eventNode());
-
- IgnitePredicate<IgniteInternalTx> replicatedOnly = tx -> {
- Collection<IgniteTxEntry> entries = tx.writeEntries();
- for (IgniteTxEntry entry : entries)
- if (cctx.cacheContext(entry.cacheId()).isReplicated())
- return true;
-
- // Checks only affected nodes contain replicated-free txs with failed primaries.
- assert partitionedRecoveryRequired;
-
- return false;
- };
-
- // Assuming that replicated transactions are absent, non-affected nodes will wait only this short sync.
- waitPartitionRelease(EXCHANGE_FREE_LATCH_ID + "-replicated", true, false, replicatedOnly);
-
- String partitionedLatchId = EXCHANGE_FREE_LATCH_ID + "-partitioned";
-
- if (partitionedRecoveryRequired)
- // This node contain backup partitions for failed partitioned caches primaries. Waiting for recovery.
- waitPartitionRelease(partitionedLatchId, true, false, null);
- else {
- // This node contain no backup partitions for failed partitioned caches primaries. Recovery is not needed.
- Latch releaseLatch = cctx.exchange().latch().getOrCreate(partitionedLatchId, initialVersion());
-
- releaseLatch.countDown(); // Await-free confirmation.
- }
- }
- }
+ waitPartitionRelease(EXCHANGE_FREE_LATCH_ID, true, false);
+ else
+ waitPartitionRelease(null, false, false);
Review comment:
Let's use some value for `latchId` (EXCHANGE_FREE_LATCH_ID). Even if there is no distributed latch created, latch id is stored to timeBag: `Wait partitions release latch [latch=...]`
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/IgniteTxManager.java
##########
@@ -918,11 +917,9 @@ public GridNearTxLocal newTx(
*
* @param topVer Topology version.
* @param node Failed node.
- * @param filter Recovery filter.
* @return Future that will be completed when all affected transactions are recovered.
*/
- public IgniteInternalFuture<Boolean> recoverLocalTxs(AffinityTopologyVersion topVer, ClusterNode node,
- IgnitePredicate<IgniteInternalTx> filter) {
+ public IgniteInternalFuture<Boolean> recoverLocalTxs(AffinityTopologyVersion topVer, ClusterNode node) {
Review comment:
Redundant NL
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/IgnitionEx.java
##########
@@ -1121,7 +1121,16 @@ private static Ignite startConfigurations(
}
if (old != null)
- if (failIfStarted) {
+ if (old.grid() == null) { // Stopped but not removed from map yet.
Review comment:
This also can happen when grid with the same name is starting concurrently, and in this case you will have 2 grid instances with the same name with access from Ignition to only one of them. It's potentially dangerous and I think much worse the previous behavior, when someone concurrently trying to start grid with the same instance name as concurrently stopped grid (all you need in this case is just wait for stop() method without concurrent start).
----------------------------------------------------------------
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] [ignite] anton-vinogradov merged pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
Posted by GitBox <gi...@apache.org>.
anton-vinogradov merged pull request #8822:
URL: https://github.com/apache/ignite/pull/8822
--
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] [ignite] anton-vinogradov commented on a change in pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
Posted by GitBox <gi...@apache.org>.
anton-vinogradov commented on a change in pull request #8822:
URL: https://github.com/apache/ignite/pull/8822#discussion_r594251160
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/IgniteTxManager.java
##########
@@ -918,11 +917,9 @@ public GridNearTxLocal newTx(
*
* @param topVer Topology version.
* @param node Failed node.
- * @param filter Recovery filter.
* @return Future that will be completed when all affected transactions are recovered.
*/
- public IgniteInternalFuture<Boolean> recoverLocalTxs(AffinityTopologyVersion topVer, ClusterNode node,
- IgnitePredicate<IgniteInternalTx> filter) {
+ public IgniteInternalFuture<Boolean> recoverLocalTxs(AffinityTopologyVersion topVer, ClusterNode node) {
Review comment:
Fixed
----------------------------------------------------------------
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] [ignite] anton-vinogradov commented on a change in pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
Posted by GitBox <gi...@apache.org>.
anton-vinogradov commented on a change in pull request #8822:
URL: https://github.com/apache/ignite/pull/8822#discussion_r594250356
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/preloader/GridDhtPartitionsExchangeFuture.java
##########
@@ -1634,41 +1631,12 @@ private void distributedExchange() throws IgniteCheckedException {
boolean skipWaitOnLocalJoin = localJoinExchange()
&& cctx.exchange().latch().canSkipJoiningNodes(initialVersion());
- if (context().exchangeFreeSwitch() && isBaselineNodeFailed()) {
+ if (context().exchangeFreeSwitch() && isBaselineNodeFailed())
Review comment:
Fixed
----------------------------------------------------------------
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] [ignite] anton-vinogradov commented on a change in pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
Posted by GitBox <gi...@apache.org>.
anton-vinogradov commented on a change in pull request #8822:
URL: https://github.com/apache/ignite/pull/8822#discussion_r604732992
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/IgniteTxManager.java
##########
@@ -939,11 +935,13 @@ public GridNearTxLocal newTx(
});
for (IgniteInternalTx tx : activeTransactions()) {
- if (tx.dht() && !tx.local() && tx.originatingNodeId().equals(node.id())) {
+ if (tx.state() == PREPARED /* recovery may be needed */
Review comment:
@alex-plekhanov had a similar concern, but about the PREPARING state.
Fixed to any state, because we should wait for the finishing of everything related to the failed node.
--
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] [ignite] anton-vinogradov commented on a change in pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
Posted by GitBox <gi...@apache.org>.
anton-vinogradov commented on a change in pull request #8822:
URL: https://github.com/apache/ignite/pull/8822#discussion_r595213990
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/IgnitionEx.java
##########
@@ -1121,7 +1121,16 @@ private static Ignite startConfigurations(
}
if (old != null)
- if (failIfStarted) {
+ if (old.grid() == null) { // Stopped but not removed from map yet.
Review comment:
Fixed by https://github.com/apache/ignite/pull/8822/commits/c691dc0c7025a18f96610b34d402566e5609cdbd
----------------------------------------------------------------
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] [ignite] anton-vinogradov commented on a change in pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
Posted by GitBox <gi...@apache.org>.
anton-vinogradov commented on a change in pull request #8822:
URL: https://github.com/apache/ignite/pull/8822#discussion_r604733557
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/IgniteTxManager.java
##########
@@ -939,11 +935,13 @@ public GridNearTxLocal newTx(
});
for (IgniteInternalTx tx : activeTransactions()) {
- if (tx.dht() && !tx.local() && tx.originatingNodeId().equals(node.id())) {
+ if (tx.state() == PREPARED /* recovery may be needed */
+ // (primary (not on originating) or backup) || (primary on originating).
+ && (tx.dht() || (tx.near() && tx.local() && ((GridNearTxLocal)tx).colocatedLocallyMapped()))
Review comment:
Unfortunately, we have to use this overcomplicated statement :(
1) We should wait for all tx's (having failed node as primary for one of tx's keys) recovery finish before finishing the switch.
Explanation of the tx flags:
- regular dth txs (txs at primary or backup, started from other node or from backup).
- near & local & colocatedLocallyMapped, txs started from some tx key's primary.
We have optimization to avoid dht tx creation in that case.
So, the pattern is `tx.dht() || (tx.near() && tx.local() && ((GridNearTxLocal)tx).colocatedLocallyMapped()` :(
Any hints are welcome!
2) Why not just wait for any type of tx?
We should avoid waiting from `tx.near() && tx.local()` but not `((GridNearTxLocal)tx).colocatedLocallyMapped()` to allow alive cells finish their switches asap without waiting for `local & near & !collocated` txs related to the failed node.
We may use the following statement instead:
`!(tx.near() && tx.local() && !((GridNearTxLocal)tx).colocatedLocallyMapped()`
but this looks less readable to me, than the current statement.
Both cases, checked by tests included in this PR
- GridExchangeFreeCellularSwitchIsolationTest # testMutliKeyTxRecoveryHappenBeforeTheSwitchOnCellularSwitch, checks we able to read the values for the keys participating in tx only after the recovery happen.
- GridExchangeFreeCellularSwitchIsolationTest # testOnlyAffectedNodesWaitForRecovery [Started from = ALIVE_CELL], checks alive cell finished the switch asap even in case tx was started from alive cell.
--
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] [ignite] anton-vinogradov commented on a change in pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
Posted by GitBox <gi...@apache.org>.
anton-vinogradov commented on a change in pull request #8822:
URL: https://github.com/apache/ignite/pull/8822#discussion_r594298902
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/distributed/dht/preloader/GridDhtPartitionsExchangeFuture.java
##########
@@ -1634,41 +1631,12 @@ private void distributedExchange() throws IgniteCheckedException {
boolean skipWaitOnLocalJoin = localJoinExchange()
&& cctx.exchange().latch().canSkipJoiningNodes(initialVersion());
- if (context().exchangeFreeSwitch() && isBaselineNodeFailed()) {
+ if (context().exchangeFreeSwitch() && isBaselineNodeFailed())
// Currently MVCC does not support operations on partially switched cluster.
if (cctx.kernalContext().coordinators().mvccEnabled())
- waitPartitionRelease(EXCHANGE_FREE_LATCH_ID, true, false, null);
- else {
- boolean partitionedRecoveryRequired = rebalancedInfo.primaryNodes.contains(firstDiscoEvt.eventNode());
-
- IgnitePredicate<IgniteInternalTx> replicatedOnly = tx -> {
- Collection<IgniteTxEntry> entries = tx.writeEntries();
- for (IgniteTxEntry entry : entries)
- if (cctx.cacheContext(entry.cacheId()).isReplicated())
- return true;
-
- // Checks only affected nodes contain replicated-free txs with failed primaries.
- assert partitionedRecoveryRequired;
-
- return false;
- };
-
- // Assuming that replicated transactions are absent, non-affected nodes will wait only this short sync.
- waitPartitionRelease(EXCHANGE_FREE_LATCH_ID + "-replicated", true, false, replicatedOnly);
-
- String partitionedLatchId = EXCHANGE_FREE_LATCH_ID + "-partitioned";
-
- if (partitionedRecoveryRequired)
- // This node contain backup partitions for failed partitioned caches primaries. Waiting for recovery.
- waitPartitionRelease(partitionedLatchId, true, false, null);
- else {
- // This node contain no backup partitions for failed partitioned caches primaries. Recovery is not needed.
- Latch releaseLatch = cctx.exchange().latch().getOrCreate(partitionedLatchId, initialVersion());
-
- releaseLatch.countDown(); // Await-free confirmation.
- }
- }
- }
+ waitPartitionRelease(EXCHANGE_FREE_LATCH_ID, true, false);
+ else
+ waitPartitionRelease(null, false, false);
Review comment:
Relocated time bag message
https://github.com/apache/ignite/pull/8822/commits/c823f58e7660b90971c63fbbfa67e7120de26ab0
----------------------------------------------------------------
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] [ignite] anton-vinogradov commented on a change in pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
Posted by GitBox <gi...@apache.org>.
anton-vinogradov commented on a change in pull request #8822:
URL: https://github.com/apache/ignite/pull/8822#discussion_r594249554
##########
File path: modules/core/src/test/java/org/apache/ignite/testframework/junits/common/GridCommonAbstractTest.java
##########
@@ -2419,6 +2422,35 @@ protected void assertCountersSame(int partId, boolean withReserveCntr) throws As
}
}
+ /**
+ * @param partId Partition.
+ * @param withReserveCntr {@code True} to compare reserve counters. Because reserve counters are synced during
+ * @param cacheName Cache name.
+ * @param cnt Counter.
+ * @param reserved Reserved counter.
+ * PME invoking with {@code true} makes sense only after PME was finished.
+ */
+ protected void assertCountersAsExpected(int partId, boolean withReserveCntr, String cacheName, long cnt,
+ long reserved) throws AssertionFailedError {
+ List<T3<String, @Nullable PartitionUpdateCounter, Boolean>> cntrMap = G.allGrids().stream().filter(ignite ->
+ !ignite.configuration().isClientMode()).map(ignite ->
+ new T3<>(ignite.name(), counter(partId, cacheName, ignite.name()),
+ ignite.affinity(cacheName).isPrimary(ignite.cluster().localNode(), partId))).collect(toList());
+
+ for (T3<String, PartitionUpdateCounter, Boolean> cntr : cntrMap) {
+ if (cntr.get2() == null)
+ continue;
+
+ assertEquals("Expecting same counters [partId=" + partId +
+ ", cntrs=" + cntrMap + ']', cnt, cntr.get2().get());
+
+ if (withReserveCntr)
Review comment:
Fixed
----------------------------------------------------------------
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] [ignite] ascherbakoff commented on a change in pull request #8822: IGNITE-13873 Milti-cell transaction changes may be not visible (durin…
Posted by GitBox <gi...@apache.org>.
ascherbakoff commented on a change in pull request #8822:
URL: https://github.com/apache/ignite/pull/8822#discussion_r598602740
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/IgniteTxManager.java
##########
@@ -939,11 +935,13 @@ public GridNearTxLocal newTx(
});
for (IgniteInternalTx tx : activeTransactions()) {
- if (tx.dht() && !tx.local() && tx.originatingNodeId().equals(node.id())) {
+ if (tx.state() == PREPARED /* recovery may be needed */
Review comment:
This doesn't look correct to me. The transaction can be already in COMMITTING state, resulting in the exchange future finished before counters are applied due to race, and broken invariant HWM >= LWM as a consequence.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/cache/transactions/IgniteTxManager.java
##########
@@ -939,11 +935,13 @@ public GridNearTxLocal newTx(
});
for (IgniteInternalTx tx : activeTransactions()) {
- if (tx.dht() && !tx.local() && tx.originatingNodeId().equals(node.id())) {
+ if (tx.state() == PREPARED /* recovery may be needed */
+ // (primary (not on originating) or backup) || (primary on originating).
+ && (tx.dht() || (tx.near() && tx.local() && ((GridNearTxLocal)tx).colocatedLocallyMapped()))
Review comment:
This condition doesn't make sense to me. Looks like it can be replaced with
&& tx.dht()
--
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