-
Notifications
You must be signed in to change notification settings - Fork 1.9k
IGNITE-27678 Same partitions on different nodes can hold different updates if writeThrough is enabled #12666
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
base: master
Are you sure you want to change the base?
Conversation
…dates if writeThrough is enabled
| res.add(nodeId); | ||
|
|
||
| return res; | ||
| return nearNodeId != nodeId ? List.of(nearNodeId, nodeId) : List.of(nearNodeId); |
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.
not relates to an issue, just a bit improvement
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.
Are we sure we need no equals()?
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.
changed
| * <ul> | ||
| * <li>Start 3 node [node0, node1, node2].</li> | ||
| * <li>Initialize put operation into transactional cache where [node1] holds primary partition for such insertion.</li> | ||
| * <li>Kill [node1] right after tx PREPARE stage is completed (it triggers tx recovery procedure.</li> |
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.
Minor javadoc fix: procedure. -> procedure).
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.
done
|
|
||
| /** Test scenario: | ||
| * <ul> | ||
| * <li>Start 3 node [node0, node1, node2].</li> |
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.
Minor javadoc fix: 3 node -> 3 nodes
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.
done
|
|
||
| /** */ | ||
| public class IdleVerifyCheckWithWriteThroughTest extends GridCommandHandlerClusterPerMethodAbstractTest { | ||
| /** */ |
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.
/** */ -> /** */
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.
done
| if (withPersistence) | ||
| cleanPersistenceDir(); | ||
|
|
||
| err = new AtomicReference<>(); |
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.
Can we initialize all there fields with their declarations.
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.
no we can`t because they are static and need to reinitialize before each test call
| }); | ||
|
|
||
| MapCacheStoreStrategy strategy = new MapCacheStoreStrategy(); | ||
| strategy.resetStore(); |
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.
Do we need to reset at start?
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.
removed
| public static Boolean failOnSessionStart; | ||
|
|
||
| /** */ | ||
| @Parameterized.Parameters(name = "cmdHnd={0}, withPersistence={1}") |
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.
and , failOnSessionStart={2} ?
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.
done, thanks
| res.add(nodeId); | ||
|
|
||
| return res; | ||
| return nearNodeId != nodeId ? List.of(nearNodeId, nodeId) : List.of(nearNodeId); |
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.
Are we sure we need no equals()?
| } | ||
| } | ||
| catch (RuntimeException e) { | ||
| U.error(log, "Exception raised during notify SessionListeners: ", 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.
Suggestion: notify SessionListeners: -> notifying cache store session listeners.
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.
changed
| protected boolean alwaysKeepBinary; | ||
|
|
||
| /** Failure handler reaction. */ | ||
| private Consumer<Throwable> failureHandlerAction; |
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.
Lets make an abbreviation.
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 do we need abbr if it correctly passed through check style ? Upper i see no abbr, also i think that abbr make it less readable.
| GridKernalContext ctx = igniteContext(); | ||
| CacheConfiguration cfg = cacheConfiguration(); | ||
|
|
||
| failureHandlerAction = e -> ctx.failure().process(new FailureContext(FailureType.CRITICAL_ERROR, 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.
Maybe we should check if(sesLsnrs!=null). No need to worry user if there are no cache store session listeners.
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.
removed
| private static CountDownLatch nodeKill; | ||
|
|
||
| /** Tx message flag. */ | ||
| private static volatile boolean finalTxMsgPassed; |
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.
it used in static class
| private static volatile boolean finalTxMsgPassed; | ||
|
|
||
| /** Session method flag. */ | ||
| private static AtomicBoolean sessionTriggered = new AtomicBoolean(); |
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.
I prefer to store as is.
...ontrol-utility/src/test/java/org/apache/ignite/util/IdleVerifyCheckWithWriteThroughTest.java
Show resolved
Hide resolved
| } | ||
| } | ||
| catch (RuntimeException e) { | ||
| U.error(log, "Exception raised during sessionEnd: ", 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.
Let's make the massage more user-friendly. What does it mean, what are the issues?
...ontrol-utility/src/test/java/org/apache/ignite/util/IdleVerifyCheckWithWriteThroughTest.java
Show resolved
Hide resolved
|




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
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see TC.Bot: Check PR)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.