You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Александр Меньшиков <sh...@gmail.com> on 2017/01/18 13:48:36 UTC
Re: Sort nodes in the ring in order to minimize the number of reconnections
I done that things:
-- Add to TcpDiscoverySpi field Comparator<TcpDiscoveryNode> nodeComparator
for load custom comparators from config file like bean.
-- Add implementation with old behavior: BaseNodeComparator
-- Add region id implementation: RegionNodeComparator which get map from IP
address to region ID in constructor.
-- Modified TcpDiscoveryNodesRing#nextNode for using nodeComparator for
find next node.
You can see that in PR: https://github.com/apache/ignite/pull/1436
Main question is: how to test it?
For my local test i just changed BaseNodeComparator with this odd
comparator:
new Comparator<TcpDiscoveryNode>() {
@Override
public int compare(TcpDiscoveryNode t1, TcpDiscoveryNode
t2) {
//shuffle nodes
final int ans =
Long.compare((t1.internalOrder()*3L+13L)%4L,
(t2.internalOrder()*3L+13L)%4L);
return (ans==0)?t1.compareTo(t2):ans;
}
};
It's looking scary, but in fact it just consistently shuffle nodes. If you
have 4 nodes with topology versions 1, 2, 3 and 4, it will be ring: 1-4-3-2.
So I think if we just using in old test this shuffle comparator and nothing
gone wrong it's good enough.
But any way I don't know how to add that to tests.
And may be we need some test for custom comparators. But in fact comparators
just must be valid Java comparator and work the same on all nodes.
Any comments are welcome.
Re: Sort nodes in the ring in order to minimize the number of reconnections
Posted by Dmitriy Setrakyan <ds...@apache.org>.
I agree with Yakov. Having an integer as region ID should be sufficient to
support all the use cases.
On Thu, Jan 19, 2017 at 4:37 AM, Yakov Zhdanov <yz...@apache.org> wrote:
> Alexander, as far as I remember we talked about having cluster id set via
> TcpDiscoverySpi configuration and also via system property.
>
> What do you mean by "existence of sufficient comparator"? If we require
> that attribute is integer then we don't have any problems. If it is not
> integer then you should throw exception on start.
>
> I repeat this once again - we need to (1)prevent our users from falling
> into terrible discovery issues (which are hard to debug) if users provide
> inconsistent comparators on different nodes, but we also need to have
> (2)full flexibility and control over nodes ordering. I think if we don't
> have comparator on public API then we are still OK with both points above.
> Therefore, I would like you to implement this feature using the approach we
> agreed on before. Let me know if you want me to take a look at the code
> before you proceed.
>
> --Yakov
>
Re: Sort nodes in the ring in order to minimize the number of reconnections
Posted by Yakov Zhdanov <yz...@apache.org>.
Guys, I have just commented in the ticket.
I suggest to unschedule IGNITE-4501 from 2.1. Let's return to it at some
point.
Thanks!
--Yakov
Re: Sort nodes in the ring in order to minimize the number of reconnections
Posted by Александр Меньшиков <sh...@gmail.com>.
Need to do code review until February 17, if we want to get this feature in
version 1.9.
2017-02-08 22:14 GMT+03:00 Александр Меньшиков <sh...@gmail.com>:
> Done. Please look.
>
> JIRA: https://issues.apache.org/jira/browse/IGNITE-4501
> PR: https://github.com/apache/ignite/pull/1436/files
> Tests: http://ci.ignite.apache.org/project.html?projectId=IgniteTes
> ts&tab=projectOverview&branch_IgniteTests=pull/1436/head
>
Re: Sort nodes in the ring in order to minimize the number of reconnections
Posted by Александр Меньшиков <sh...@gmail.com>.
Done. Please look.
JIRA: https://issues.apache.org/jira/browse/IGNITE-4501
PR: https://github.com/apache/ignite/pull/1436/files
Tests: http://ci.ignite.apache.org/project.html?projectId=IgniteTes
ts&tab=projectOverview&branch_IgniteTests=pull/1436/head
Re: Sort nodes in the ring in order to minimize the number of reconnections
Posted by Александр Меньшиков <sh...@gmail.com>.
Igor, I have thought about approach what you are talking about. It need add
new field named like "sortedNodes" with custom ordering, which will have
the same items as "nodes" field, because "nodes" has being used with
default ordering in other methods. It have this advantages:
1. Method "nextNode" will look simpler.
2. Method "nextNode" will work faster, because using of method
TreeSet#higher() will be available. But that possibility had not been used
in original code. And I don't why.
But also have some disadvantages because new field "sortedNodes" will be
strongly connected with "nodes":
1. It need copy-paste all code, which modifies "nodes" in 4 other methods.
It will decrease maintainability.
2. Field "nodes" is being used with "copy-on-write" algorithm. So state of
"nodes" and "sortedNodes" can be inconsistent. Maybe it's okay, in fact I
just don't know. But any way in future it may become a problem.
So my opinion is that "presorted" approach can work a little bit faster
(number of nodes never can't be so big that O(log n) became more faster
than O(n)), but code complexity will been increased, because it will add
one logic connection inside the whole class "TcpDiscoveryNodesRing".
Yakov, can you settle our argument?
2017-01-20 16:30 GMT+03:00 Игорь Г <fr...@gmail.com>:
> Alexander, maybe you should use presorted collection in
> TcpDiscoveryNodesRing.nextNode instead of iterating through unsorted one
> every time?
>
Re: Sort nodes in the ring in order to minimize the number of reconnections
Posted by Игорь Г <fr...@gmail.com>.
Alexander, maybe you should use presorted collection in
TcpDiscoveryNodesRing.nextNode instead of iterating through unsorted one
every time?
Re: Sort nodes in the ring in order to minimize the number of reconnections
Posted by Александр Меньшиков <sh...@gmail.com>.
Yakov, I changed the implementation. Please, look at my code again.
https://github.com/apache/ignite/pull/1436
2017-01-19 15:37 GMT+03:00 Yakov Zhdanov <yz...@apache.org>:
> Alexander, as far as I remember we talked about having cluster id set via
> TcpDiscoverySpi configuration and also via system property.
>
> What do you mean by "existence of sufficient comparator"? If we require
> that attribute is integer then we don't have any problems. If it is not
> integer then you should throw exception on start.
>
> I repeat this once again - we need to (1)prevent our users from falling
> into terrible discovery issues (which are hard to debug) if users provide
> inconsistent comparators on different nodes, but we also need to have
> (2)full flexibility and control over nodes ordering. I think if we don't
> have comparator on public API then we are still OK with both points above.
> Therefore, I would like you to implement this feature using the approach we
> agreed on before. Let me know if you want me to take a look at the code
> before you proceed.
>
> --Yakov
>
Re: Sort nodes in the ring in order to minimize the number of reconnections
Posted by Yakov Zhdanov <yz...@apache.org>.
Alexander, as far as I remember we talked about having cluster id set via
TcpDiscoverySpi configuration and also via system property.
What do you mean by "existence of sufficient comparator"? If we require
that attribute is integer then we don't have any problems. If it is not
integer then you should throw exception on start.
I repeat this once again - we need to (1)prevent our users from falling
into terrible discovery issues (which are hard to debug) if users provide
inconsistent comparators on different nodes, but we also need to have
(2)full flexibility and control over nodes ordering. I think if we don't
have comparator on public API then we are still OK with both points above.
Therefore, I would like you to implement this feature using the approach we
agreed on before. Let me know if you want me to take a look at the code
before you proceed.
--Yakov
Re: Sort nodes in the ring in order to minimize the number of reconnections
Posted by Александр Меньшиков <sh...@gmail.com>.
Yakov, as I understand it we need add CLUSTER_REGION_ID for each nodes in
config file. And in fact using some kind of sort in nextNode method (the
search for extreme values to be exact). And the existence of valid
comparator is a sufficient condition to sort nodes to build new correct
ring. So I has thought we will not get any extra benefits (performance or
maintainability) if we close the ability for users to set their sort logic.
Code will a similar in two variants. I has thought if I show this variant
will be easier to see that variant is okay.
But if not, then I can fast change code.
Re: Sort nodes in the ring in order to minimize the number of reconnections
Posted by Yakov Zhdanov <yz...@apache.org>.
Alexander, I was against any comparator and user defined logic exactly for
reason that comparison may be inconsistent. After long discussion and
consensus you implement approach with comparator. Can you please explain
why you did not just add logic to compare the value of CLUSTER_REGION_ID
node attribute?
--Yakov
2017-01-18 16:48 GMT+03:00 Александр Меньшиков <sh...@gmail.com>:
> I done that things:
>
> -- Add to TcpDiscoverySpi field Comparator<TcpDiscoveryNode>
> nodeComparator for load custom comparators from config file like bean.
> -- Add implementation with old behavior: BaseNodeComparator
> -- Add region id implementation: RegionNodeComparator which get map from
> IP address to region ID in constructor.
> -- Modified TcpDiscoveryNodesRing#nextNode for using nodeComparator for
> find next node.
>
> You can see that in PR: https://github.com/apache/ignite/pull/1436
>
> Main question is: how to test it?
>
> For my local test i just changed BaseNodeComparator with this odd
> comparator:
>
> new Comparator<TcpDiscoveryNode>() {
> @Override
> public int compare(TcpDiscoveryNode t1, TcpDiscoveryNode
> t2) {
> //shuffle nodes
> final int ans = Long.compare((t1.internalOrder()*3L+13L)%4L,
> (t2.internalOrder()*3L+13L)%4L);
> return (ans==0)?t1.compareTo(t2):ans;
> }
> };
>
> It's looking scary, but in fact it just consistently shuffle nodes. If you
> have 4 nodes with topology versions 1, 2, 3 and 4, it will be ring: 1-4-3-2.
>
> So I think if we just using in old test this shuffle comparator and
> nothing gone wrong it's good enough.
>
> But any way I don't know how to add that to tests.
>
> And may be we need some test for custom comparators. But in fact comparators
> just must be valid Java comparator and work the same on all nodes.
>
> Any comments are welcome.
>