10:38 a.m.
Hi all,
I've noticed a problem with the way we test state transfer in our testsuite. For
example, take
https://github.com/infinispan/infinispan/blob/master/core/src/test/java/o...
This test checks that when a new node is started, state transfer happens. But, it could
happen that a merge happens instead of a join, so if a merge happens no state transfer
occurs.
Now, the problem is that waiting for the view to be set happens in the main thread, and
the callback to merge view listener happens in a different thread. So, in an unfortunate
situation, the following can happen:
1. [main-thread] waits for view to be set.
2. [main-thread] view is set due to a merge and main thread carries on.
3. [main-thread] checks the merge view listener and sees that
4. [callback-thread] calls MergedViewListener.mergedView and sets merged=true.
I've seen this failure happening in my local machine when trying to replicate other
random failures.
So, I'm solving this issue by having a listener that listens for both merge and view
changes, and then having a latch that can waits for either one of the two callbacks to
happen. Clearly, the countDown() would happen after either merged (boolean) or
viewChanged(boolean) have been set, so that gives the guarantees that either a merge
happened or not and then I can check the initial data if necessary. I'll send a pull
req later on today with this.
Btw, you might be wondering how on earth a merge would happen with our new TEST_PING? I
have that question too and seems like sometimes Discovery.sendGetMembersRequest does not
get called and my TEST_PING implementation assumes that will definitely get called.
I've sent an email to Bela and I'm gonna try to add some more debugging to find
out how on earth this happens.
Now, what I'm wondering here is whether this is something the end users would be
interested as well cos they might be running their own testing to check whether state
transfer works as expected.
It's at this point that I miss tuples in java cos I think it'd be handy to have a
getMembers() call that returns not only a List<Address> but also an enum value
indicating whether the last view change was a merge or view change, or more simply a
boolean indicating whether the last view change was a merge or not:
(boolean, List<Members>) getMembers();
Unfortunately Java does not make it easy to return things like this. Having a separate
method to find out if the last view change was a merge or not would be clunky cos
you'd want a single method that can provide the guarantees with regards to the member
list returned.
Any thoughts on this API enhancement? Would it be useful? In the Java world, it would
require creating a new type which is a bit of a deterrent for me.
--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache
3:19 p.m.
On 11-07-19 4:38 AM, Galder Zamarreño wrote:
Btw, you might be wondering how on earth a merge would happen with
our new TEST_PING? I have that question too and seems like sometimes
Discovery.sendGetMembersRequest does not get called and my TEST_PING implementation
assumes that will definitely get called. I've sent an email to Bela and I'm gonna
try to add some more debugging to find out how on earth this happens.
I saw your emails. Keep us posted please. It might be a bug during
channel startup?
Now, what I'm wondering here is whether this is something the end
users would be interested as well cos they might be running their own testing to check
whether state transfer works as expected.
It's at this point that I miss tuples in java cos I think it'd be handy to have a
getMembers() call that returns not only a List<Address> but also an enum value
indicating whether the last view change was a merge or view change, or more simply a
boolean indicating whether the last view change was a merge or not:
(boolean, List<Members>) getMembers();
Unfortunately Java does not make it easy to return things like this. Having a separate
method to find out if the last view change was a merge or not would be clunky cos
you'd want a single method that can provide the guarantees with regards to the member
list returned.
Any thoughts on this API enhancement? Would it be useful? In the Java world, it would
require creating a new type which is a bit of a deterrent for me.
This could be added into a View I suppose, but that does not help you :-)
Regards,
Vladimir
3:54 p.m.
On Jul 19, 2011, at 3:19 PM, Vladimir Blagojevic wrote:
On 11-07-19 4:38 AM, Galder Zamarreño wrote:
> Btw, you might be wondering how on earth a merge would happen with our new TEST_PING?
I have that question too and seems like sometimes Discovery.sendGetMembersRequest does not
get called and my TEST_PING implementation assumes that will definitely get called.
I've sent an email to Bela and I'm gonna try to add some more debugging to find
out how on earth this happens.
I saw your emails. Keep us posted please. It might be a bug during channel startup?
Ever since I fixed the StateTransferFunctionalTest as mentioned earlier, I haven't
been able to replicate it. So, I'm heading down to master to try there with the added
log messages in Discovery.
Judging by the fact that the wait happens, I reckon that this condition returns false in
Discovery:
if(senderFuture == null || senderFuture.isDone())
The only way I can see this happening is if two channels are sharing the same
Discovery/TEST_PING instance.
I'll keep running the test to see if I can get it to show this behaviour, but it's
quite hard.
> Now, what I'm wondering here is whether this is something the end users would be
interested as well cos they might be running their own testing to check whether state
transfer works as expected.
>
> It's at this point that I miss tuples in java cos I think it'd be handy to
have a getMembers() call that returns not only a List<Address> but also an enum
value indicating whether the last view change was a merge or view change, or more simply a
boolean indicating whether the last view change was a merge or not:
>
> (boolean, List<Members>) getMembers();
>
> Unfortunately Java does not make it easy to return things like this. Having a
separate method to find out if the last view change was a merge or not would be clunky cos
you'd want a single method that can provide the guarantees with regards to the member
list returned.
>
> Any thoughts on this API enhancement? Would it be useful? In the Java world, it would
require creating a new type which is a bit of a deterrent for me.
This could be added into a View I suppose, but that does not help you :-)
Yeah, doesn't help cos we can already figure that out as seen in
JGroupsTransport.viewAccepted class.
Regards,
Vladimir
--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache
5:36 p.m.
On 19 Jul 2011, at 09:38, Galder Zamarreño wrote:
Hi all,
I've noticed a problem with the way we test state transfer in our testsuite. For
example, take
https://github.com/infinispan/infinispan/blob/master/core/src/test/java/o...
This test checks that when a new node is started, state transfer happens. But, it could
happen that a merge happens instead of a join, so if a merge happens no state transfer
occurs.
that was my understanding as well. And I think it still stands for REPL.
I've just had a chat with Dan and also looked at the code[1]: for distributed caches,
if a merge view happens, the rehashing is triggered in exactly the same way as when a join
happens. That worries me, as consistency is affected if a key is modified in one (or both)
of the cluster's partition after the split brain. Or am I missing something?
[1] http://bit.ly/o9Cx99
Now, the problem is that waiting for the view to be set happens in the main thread, and
the callback to merge view listener happens in a different thread. So, in an unfortunate
situation, the following can happen:
1. [main-thread] waits for view to be set.
2. [main-thread] view is set due to a merge and main thread carries on.
3. [main-thread] checks the merge view listener and sees that
4. [callback-thread] calls MergedViewListener.mergedView and sets merged=true.
I've seen this failure happening in my local machine when trying to replicate other
random failures.
So, I'm solving this issue by having a listener that listens for both merge and view
changes, and then having a latch that can waits for either one of the two callbacks to
happen.
Clearly, the countDown() would happen after either merged (boolean) or
viewChanged(boolean) have been set, so that gives the guarantees that either a merge
happened or not and then I can check the initial data if necessary. I'll send a pull
req later on today with this.
Btw, you might be wondering how on earth a merge would happen with our new TEST_PING? I
have that question too and seems like sometimes Discovery.sendGetMembersRequest does not
get called and my TEST_PING implementation assumes that will definitely get called.
I've sent an email to Bela and I'm gonna try to add some more debugging to find
out how on earth this happens.
I think that even if you fix this in the unit tests
it still might happen in a real-life situation, i.e. start two nodes and instead of
forming a cluster they'd first form two clusters and then merge.
Now, what I'm wondering here is whether this is something the end users would be
interested as well cos they might be running their own testing to check whether state
transfer works as expected.
Thinking loud about this issue: can't jgroups
realise that this node wants to join and not to merge? E.g. if node B starts and wants to
join cluster {A}: if B hasn't received any application level messages than can't
jgroups just assume that it definitely wants to join, and never wants to merge?
It's at this point that I miss tuples in java cos I think it'd be handy to have a
getMembers() call that returns not only a List<Address> but also an enum value
indicating whether the last view change was a merge or view change, or more simply a
boolean indicating whether the last view change was a merge or not:
(boolean, List<Members>) getMembers();
Unfortunately Java does not make it easy to return things like this. Having a separate
method to find out if the last view change was a merge or not would be clunky cos
you'd want a single method that can provide the guarantees with regards to the member
list returned.
Any thoughts on this API enhancement? Would it be useful? In the Java world, it would
require creating a new type which is a bit of a deterrent for me.
I'm not sure
how useful this information is *after* the event (be it merge or join) took place. Beside
this very specific use case, I cannot think of another in which a user wants to know the
type of view change *after* it took place..
--
Galder Zamarreño
Sr. Software Engineer
Infinispan, JBoss Cache
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
6:35 p.m.
On 7/19/11 5:36 PM, Mircea Markus wrote:
On 19 Jul 2011, at 09:38, Galder Zamarreño wrote:
> Hi all,
>
> I've noticed a problem with the way we test state transfer in our testsuite. For
example, take
https://github.com/infinispan/infinispan/blob/master/core/src/test/java/o...
>
> This test checks that when a new node is started, state transfer happens. But, it
could happen that a merge happens instead of a join, so if a merge happens no state
transfer occurs.
that was my understanding as well. And I think it still stands for REPL.
I've just had a chat with Dan and also looked at the code[1]: for distributed caches,
if a merge view happens, the rehashing is triggered in exactly the same way as when a join
happens.
That worries me, as consistency is affected if a key is modified in one (or both) of the
cluster's partition after the split brain. Or am I missing something?
I wrote that to reduce code; the code to join is essentially the same as
the code to merge. For me, this is the same.
Now, there *is* a problem. Consider we have partitions {A} and {B}, and
some clients are able to access A and others B. Now value x (which is 1)
is set to 2 in B.
When there's a merge into {A,B} and A turns out to be the (existing and
new) primary owner of x, then it'll update B.x=1 (*if* B is the backup
owner of x, or else B will remove x).
If B turns out to be the primary owner, then it will update A.x=2 (*if*
A is the backup owner for x).
Since we have never handled merge situations, neither in JBossCache, not
in Infinispan, I think we're fine for me. Of course, this needs to be
addressed, but that's a separate discussion (albeit a very important one
!)...
We could use seqnos and let the user decide, on a merge, which value is
the correct one, unless we can merge them without conflicts.
--
Bela Ban
Lead JGroups (http://www.jgroups.org)
JBoss / Red Hat
6:49 p.m.
On 19 Jul 2011, at 17:35, Bela Ban wrote:
On 7/19/11 5:36 PM, Mircea Markus wrote:
>
> On 19 Jul 2011, at 09:38, Galder Zamarreño wrote:
>
>> Hi all,
>>
>> I've noticed a problem with the way we test state transfer in our testsuite.
For example, take
https://github.com/infinispan/infinispan/blob/master/core/src/test/java/o...
>>
>> This test checks that when a new node is started, state transfer happens. But, it
could happen that a merge happens instead of a join, so if a merge happens no state
transfer occurs.
> that was my understanding as well. And I think it still stands for REPL.
> I've just had a chat with Dan and also looked at the code[1]: for distributed
caches, if a merge view happens, the rehashing is triggered in exactly the same way as
when a join happens.
> That worries me, as consistency is affected if a key is modified in one (or both) of
the cluster's partition after the split brain. Or am I missing something?
I wrote that to reduce code; the code to join is essentially the same as
the code to merge. For me, this is the same.
Now, there *is* a problem. Consider we have partitions {A} and {B}, and
some clients are able to access A and others B. Now value x (which is 1)
is set to 2 in B.
When there's a merge into {A,B} and A turns out to be the (existing and
new) primary owner of x, then it'll update B.x=1 (*if* B is the backup
owner of x, or else B will remove x).
This is precisely the thing that worries me:
with previous code B.x=2 didn't get lost. User's @Merge listener were called
instead and the user could do some stuff to merge the state, but updates were not lost.
I agree that the previous approach placed a lot of burden on user's shoulder: merge
the state "manually", but that was for a reason - not to loose consistency,
which the current approach does. Hence my surprise to see it.
If B turns out to be the primary owner, then it will update A.x=2 (*if*
A is the backup owner for x).
Since we have never handled merge situations, neither in JBossCache, not
in Infinispan, I think we're fine for me. Of course, this needs to be
addressed, but that's a separate discussion (albeit a very important one
!)...
We could use seqnos and let the user decide, on a merge, which value is
the correct one, unless we can merge them without conflicts.
--
Bela Ban
Lead JGroups (http://www.jgroups.org)
JBoss / Red Hat
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
7 p.m.
On 7/19/11 6:49 PM, Mircea Markus wrote:
> When there's a merge into {A,B} and A turns out to be the
(existing and
> new) primary owner of x, then it'll update B.x=1 (*if* B is the backup
> owner of x, or else B will remove x).
This is precisely the thing that worries me: with previous code B.x=2 didn't get
lost. User's @Merge listener were called instead and the user could do some stuff to
merge the state, but updates were not lost.
I agree that the previous approach placed a lot of burden on user's shoulder: merge
the state "manually", but that was for a reason - not to loose consistency,
which the current approach does. Hence my surprise to see it.
Wait ! The @Merged annotation just invoked a callback which gave you a
MergeEvent (similar to @View giving you a ViewEvent).
I don't think it was ever intended to be used to merge state, at least
the documentation of @Merged never mentioned this (IMO)...
I also don't think that anybody (us or users) has ever written any code
to merge data on a MergeEvent...
Note that this is probably what should be done, but we should tackle
this in more sophisticated way than throwing a callback at the user and
telling them to handle the merge.
*If* we used version numbers that were incremented on changes, we could
do some of the merging ourselves, and throw the conflicting changes to
the users to handle. Perhaps we could even provide some merge strategies
that a user could pick.
However, this is not exactly trivial, e.g. how do conflicting removes
and changes get handled, tombstones ?
This is something we should discuss separately from the new rebalancing
code. Note that this also applies to replication, not only distribution.
--
Bela Ban
Lead JGroups (http://www.jgroups.org)
JBoss / Red Hat
7:16 p.m.
On 19 Jul 2011, at 18:00, Bela Ban wrote:
On 7/19/11 6:49 PM, Mircea Markus wrote:
>> When there's a merge into {A,B} and A turns out to be the (existing and
>> new) primary owner of x, then it'll update B.x=1 (*if* B is the backup
>> owner of x, or else B will remove x).
> This is precisely the thing that worries me: with previous code B.x=2 didn't get
lost. User's @Merge listener were called instead and the user could do some stuff to
merge the state, but updates were not lost.
> I agree that the previous approach placed a lot of burden on user's shoulder:
merge the state "manually", but that was for a reason - not to loose
consistency, which the current approach does. Hence my surprise to see it.
Wait ! The @Merged annotation just invoked a callback which gave you a
MergeEvent (similar to @View giving you a ViewEvent).
I don't think it was ever intended to be used to merge state, at least
the documentation of @Merged never mentioned this (IMO)...
I also don't think that anybody (us or users) has ever written any code
to merge data on a MergeEvent...
Note that this is probably what should be done, but we should tackle
this in more sophisticated way than throwing a callback at the user and
telling them to handle the merge.
*If* we used version numbers that were incremented on changes, we could
do some of the merging ourselves, and throw the conflicting changes to
the users to handle. Perhaps we could even provide some merge strategies
that a user could pick.
However, this is not exactly trivial, e.g. how do conflicting removes
and changes get handled, tombstones ?
Agreed on the versioning for auto-merge.
I'm just wandering weather it wouldn't make more sense, for now, not to do any
(incorrect) merge as we do now, but stick with the old approach of migrating this
responsibility to the user.
When versioning is in place we can start merging things indeed, but ATM I think it's
better not to do anything than do something that creates inconsistencies.
Or make this auto-merge behaviour configurable and let the user decide?
This is something we should discuss separately from the new rebalancing
code. Note that this also applies to replication, not only distribution.
Just a
note on state transfer with replication: 5.1's state transfer (REPL) uses the
rehashing code from distribution to move state around: the basic idea is that if
numOwners>clusterSize, when a node joins you practically have state transfer.
So post 5.1, state-transfer and rebalancing are the same thing. Almost :-)
7:44 p.m.
On 7/19/11 7:16 PM, Mircea Markus wrote:
Agreed on the versioning for auto-merge. I'm just wondering
weather it wouldn't make more sense, for now, not to do any (incorrect) merge as we do
now, but stick with the old approach of
migrating this responsibility to the user.
The old approach was (1) incorrect and (2) nobody ever implemented
@Merged to merge state !
(1): for example could a joiner transfer its state (null) to an existing
coordinator (not null state) !
The old approach was incorrect. The new approach still doesn't handle
merges, but at least it is consistent.
> This is something we should discuss separately from the new
rebalancing code. Note that this also applies to replication, not only distribution.
Just a note on state transfer with replication: 5.1's state
transfer (REPL) uses the rehashing code from distribution to move state around: the basic
idea is that if numOwners>clusterSize,
when a node joins you practically have state transfer.
I see you still haven't given up on your old idea... :-)
--
Bela Ban
Lead JGroups (http://www.jgroups.org)
JBoss / Red Hat
5:38 p.m.
A quick note on @Merged: it isn't intended for use as a state merge mechanism. It is
just a notification. You can't really merge state unless you have some form of entry
versioning, and preferably a versioning scheme that is resilient to concurrent updates
such as a vector clock. And even then, you can only "fast-forward" in certain
cases, so the callback needs to be modified to handle the cases where there is a conflict.
Further, the API would need to change to handle this eventually consistent behaviour as
well (Pete and I are talking about this for JSR 347 too). Essentially an
EventuallyConsistentCache would have a signature for GET which looks a bit like:
EventuallyConsistentCache {
VersionedValue<V> get(K key) throws VersionConflictException;
void resolveVersionConflict(K key, Version correctVersion);
}
VersionConflictException extends CacheException {
Set<VersionedValue<V>> getConflictingVersions();
}
VersionedValue<V> {
V getValue();
Version getVersion();
}
Naturally, this adds overhead (memory) and performance (gc) so this eventually consistent
mode will be disabled by default.
Cheers
Manik
On 19 Jul 2011, at 18:44, Bela Ban wrote:
On 7/19/11 7:16 PM, Mircea Markus wrote:
> Agreed on the versioning for auto-merge. I'm just wondering weather it
wouldn't make more sense, for now, not to do any (incorrect) merge as we do now, but
stick with the old approach of
> migrating this responsibility to the user.
The old approach was (1) incorrect and (2) nobody ever implemented
@Merged to merge state !
(1): for example could a joiner transfer its state (null) to an existing
coordinator (not null state) !
The old approach was incorrect. The new approach still doesn't handle
merges, but at least it is consistent.
>> This is something we should discuss separately from the new rebalancing code.
Note that this also applies to replication, not only distribution.
> Just a note on state transfer with replication: 5.1's state transfer (REPL) uses
the rehashing code from distribution to move state around: the basic idea is that if
numOwners>clusterSize,
> when a node joins you practically have state transfer.
I see you still haven't given up on your old idea... :-)
--
Bela Ban
Lead JGroups (http://www.jgroups.org)
JBoss / Red Hat
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
--
Manik Surtani
manik(a)jboss.org
twitter.com/maniksurtani
Lead, Infinispan
http://www.infinispan.org
6:39 p.m.
On 7/19/11 5:36 PM, Mircea Markus wrote:
I think that even if you fix this in the unit tests it still might
happen in a real-life situation, i.e. start two nodes and instead of forming a cluster
they'd first form two clusters and then merge.
Thinking loud about this issue: can't jgroups realise that this node wants to join
and not to merge? E.g. if node B starts and wants to join cluster {A}: if B hasn't
received any application level
messages than can't jgroups just assume that it definitely wants to
join, and
never wants to merge?
What if you start A, then the switch goes down, then start B ? B knows
it wants to join, but doesn't see A. A and B will only see each other
when the switch comes back up.
Of course, we always strive to have regular joins, not merges, but this
cannot always be achieved.
--
Bela Ban
Lead JGroups (http://www.jgroups.org)
JBoss / Red Hat
6:53 p.m.
On 19 Jul 2011, at 17:39, Bela Ban wrote:
On 7/19/11 5:36 PM, Mircea Markus wrote:
> I think that even if you fix this in the unit tests it still might happen in a
real-life situation, i.e. start two nodes and instead of forming a cluster they'd
first form two clusters and then merge.
> Thinking loud about this issue: can't jgroups realise that this node wants to
join and not to merge? E.g. if node B starts and wants to join cluster {A}: if B
hasn't received any application level
> messages than can't jgroups just assume that it definitely wants to
join, and never wants to merge?
What if you start A, then the switch goes down, then start B ? B knows
it wants to join, but doesn't see A. A and B will only see each other
when the switch comes back up.
Of course, we always strive to have regular joins, not merges, but this
cannot always be achieved.
Thinking some more this situation will be nicely solved
when version(vector clock) based merging will be in place, so doesn't make too much
sense to strive for joins over merges ...
4663
days inactive
4671
days old
infinispan-dev@lists.jboss.org
11 comments
5 participants
participants (5)
-
Bela Ban -
Galder Zamarreño -
Manik Surtani -
Mircea Markus -
Vladimir Blagojevic