IGNITE-26584 Discovery optimizations for MultiDC#12517
IGNITE-26584 Discovery optimizations for MultiDC#12517anton-vinogradov merged 103 commits intoapache:masterfrom
Conversation
# Conflicts: # modules/core/src/test/java/org/apache/ignite/testsuites/IgniteSpiDiscoverySelfTestSuite.java
|
|
||
| Iterator<TcpDiscoveryNode> iter = filtered.iterator(); | ||
| NavigableSet<TcpDiscoveryNode> sorted = new TreeSet<>(new MdcAwareNodesComparator()); | ||
| sorted.addAll(filtered); |
There was a problem hiding this comment.
I'm not sure if TreeSet always preserves order of added elements if all elements are equal according to comparator, so it potentianly can break something in non-multi-DC cluster. Let's use sorting only for multi-DC environment. Also it's a potentially dangerous feature even for multi-DC, I think it's worth to have a flag to disable it explicitely.
There was a problem hiding this comment.
MdcAwareNodesComparator guarantee elements to be non-equal since it compares dcId and if they are equal it compares nodes as usual.
if (res == 0)
res = n1.compareTo(n2);
There was a problem hiding this comment.
I agree that having a switch to turn this feature off and fall back to a manual management of ring structure is necessary.
There is always a chance that we miss an edge case during testing and a critical bug in discovery makes it to production. Having a flag as a way to get back to a suboptimal but working implementation is critical in such situation.
There was a problem hiding this comment.
Fixed with
private static final boolean mdcAwareRing = IgniteSystemProperties.getBoolean("MDC_AWARE_RING", true);
Collection<TcpDiscoveryNode> sorted;
if (mdcAwareRing) {
sorted = new TreeSet<>(new MdcAwareNodesComparator());
sorted.addAll(nodes);
}
else
sorted = nodes;
| String n1DcId = n1.dataCenterId() == null ? "" : n1.dataCenterId(); | ||
| String n2DcId = n2.dataCenterId() == null ? "" : n2.dataCenterId(); |
There was a problem hiding this comment.
For multi-DC environment we have a check, that all dataCenterId is provided. We can ommit nullability check if comparator will be used only for multi-DC.
There was a problem hiding this comment.
Removed the "" defaults.
| void add(TcpDiscoveryAbstractMessage msg) { | ||
| msgs.add(new PendingMessage(msg)); | ||
|
|
||
| while (msgs.size() > MAX) { |
There was a problem hiding this comment.
As far as I can see, the old implementation have this logic to maintain internal queue size and not let it get close to MAX * 2 size right on add operation.
Now there is no such logic, we clean up internal collection on discard call itself.
Do we understand the logic behing this old behavior? What are situations when this logic starts working? Does new implementation perform better or worse in these situations?
This whole discard logic looks very important to me, and I want to make sure that this change doesn't break some corner case with lagging network which is not covered with our test base.
There was a problem hiding this comment.
Old sollution (queue limiting) seems to be a buggy. In case of a queue overfill we just lose the consistency.
New solution guarantee the consistency and cleans the elements only when it's safe to clean.
Not sure we have deployments for 1K+ nodes where this will NOT happen now, but code becomes more stable, I think.
|
3 participants
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.