-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ignite-16685 : Expiration fails on topology version when cluster gets re-activated. V2 - topology version check. #9888
Conversation
@Override protected IgniteConfiguration getConfiguration(String igniteInstanceName) throws Exception { | ||
long time = TimeUnit.SECONDS.toMillis(TIMEOUT_SEC); | ||
|
||
PlatformExpiryPolicyFactory plc = new PlatformExpiryPolicyFactory(time, time, time); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is not related to platform somehow, please use CreatedExpiryPolicy.factoryOf()
or other policy factory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
*/ | ||
public class ActivationOnExpirationTimeoutTest extends GridCommonAbstractTest { | ||
/** */ | ||
private static final int TIMEOUT_SEC = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much timeout, it takes more than 15 seconds to pass the test (I think 1/2 of second will be enough).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
super.sendMessage(node, msg, ackC); | ||
} | ||
}) | ||
.setFailureHandler(new StopNodeFailureHandler()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to check that failure handler is not triggered, it's better to create own failure handler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
assertEquals(ClusterState.ACTIVE, G.allGrids().get(0).cluster().state()); | ||
assertEquals(ClusterState.ACTIVE, G.allGrids().get(1).cluster().state()); | ||
|
||
GridTestUtils.waitForCondition(() -> cache.size() == 0, TIMEOUT_SEC); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertTrue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? The cluster is just activated. It needs some time to initialize, do PME and permit expiring to work.
private static final int TIMEOUT_SEC = 5; | ||
|
||
/** */ | ||
private static final int TOPOLOGY_UPDATE_DELAY_MILLS = 2000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too much timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -3106,7 +3106,7 @@ private int purgeExpiredInternal( | |||
return 0; | |||
|
|||
try { | |||
if (part != null && part.state() != OWNING) | |||
if (part == null || !cctx.topology().initialized()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we don't need check for OWNING state now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -82,6 +83,7 @@ | |||
GridTestUtils.addTestIfNeeded(suite, PageLockTrackerResourcesTest.class, ignoredTests); | |||
|
|||
GridTestUtils.addTestIfNeeded(suite, IgnitePdsCacheEntriesExpirationTest.class, ignoredTests); | |||
GridTestUtils.addTestIfNeeded(suite, ActivationOnExpirationTimeoutTest.class, ignoredTests); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ActivationOnExpirationTimeoutTest
uses atomic cache, why do we need MVCC test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
try { | ||
U.sleep(TOPOLOGY_UPDATE_DELAY_MILLS); | ||
} | ||
catch (IgniteInterruptedCheckedException e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e
-> ignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
.setCommunicationSpi(new TcpCommunicationSpi() { | ||
@Override public void sendMessage(ClusterNode node, Message msg, | ||
IgniteInClosure<IgniteException> ackC) throws IgniteSpiException { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant NL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
||
return super.getConfiguration(igniteInstanceName) | ||
.setCommunicationSpi(new TcpCommunicationSpi() { | ||
@Override public void sendMessage(ClusterNode node, Message msg, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong code style (each parameter should be on it's own line)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
* limitations under the License. | ||
*/ | ||
|
||
package org.apache.ignite.util; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
…pache#9888. IGNITE-16685 Fix expiration failure on cluster re-activation - Fixes apache#9888. Squashed commit of the following: commit b2c07baeb5992a8be65f6463e1ea42da1088f34f Author: vladsz83 <vladsz83@gmail.com> Date: Fri Mar 25 12:32:45 2022 +0300 IGNITE-16685 Fix expiration failure on cluster re-activation - Fixes apache#9888. Signed-off-by: Aleksey Plekhanov <plehanov.alex@gmail.com> (cherry picked from commit c4d3e5d)
…pache#9888. Signed-off-by: Aleksey Plekhanov <plehanov.alex@gmail.com>
Solution:
Skip partition if topology version isn't initialized additionally to partition state checking (state==OWNING).