7:03 p.m.
More fun with buddy replication. :-)
Saw an error on 1 of our failover tests where,
1) Node D had left the group, so lots of gravitation was going on.
2) Various nodes were sending DataGravitationCleanupCommands to the
cluster for /BUDDY_BACKUP/D_DEAD/1/JSESSION/st_localhost/xxx. Result is
all nodes in the cluster are trying to remove various
/BUDDY_BACKUP/D_DEAD/1/JSESSION/st_localhost/xxx nodes. On node A those
nodes don't exist, so PessimisticLockInterceptor.handleRemoveCommand is
adding them and removing them.
3) Concurrent with #2, a GravitateDataCommand for
/JSESSION/st_localhost/123 comes in to node A. Session 123 was never
stored on node A, so this should result in a cache miss. But what
happened once was:
[JBoss] 16:46:52,961 TRACE
[org.jboss.cache.marshall.CommandAwareRpcDispatcher]
(Incoming-13,10.34.32.153:14736) Problems invoking command.
[JBoss] org.jboss.cache.NodeNotValidException: Node
/_BUDDY_BACKUP_/10.34.32.156_48822:DEAD/1/JSESSION/st_localhost/UvzutZkoESBMRSnjv0eTRA__
is not valid. Perhaps it has been moved or removed.
[JBoss] at
org.jboss.cache.invocation.NodeInvocationDelegate.assertValid(NodeInvocationDelegate.java:527)
[JBoss] at
org.jboss.cache.invocation.NodeInvocationDelegate.getChildrenNames(NodeInvocationDelegate.java:292)
[JBoss] at
org.jboss.cache.commands.read.GravitateDataCommand.perform(GravitateDataCommand.java:176)
...
It seems the command is seeing a non-existent node. Yep; looking at the
logs it's clear the above GravitateDataCommand was executed concurrently
with another DataGravitationCleanupCommand for the same session. (I need
to investigate why that happened.)
Below is a possible patch to work around the issue. This points to a
more general locking problem though -- should these "phantom nodes"
created for removal be visible to other threads? Shouldn't there be a WL
on them from the moment they are created until after they are removed?
Hehe, answered my own question by writing it. The node is created by
PessimisticNodeBasedLockManager and then locked. There's a gap in
between where another thread could get a ref to it.
Anyway, the patch:
### Eclipse Workspace Patch 1.0
#P jbosscache-core
Index: src/main/java/org/jboss/cache/commands/read/GravitateDataCommand.java
===================================================================
---
src/main/java/org/jboss/cache/commands/read/GravitateDataCommand.java
(revision 8163)
+++
src/main/java/org/jboss/cache/commands/read/GravitateDataCommand.java
(working copy)
@@ -29,6 +29,7 @@
import org.jboss.cache.InternalNode;
import org.jboss.cache.InvocationContext;
import org.jboss.cache.Node;
+import org.jboss.cache.NodeNotValidException;
import org.jboss.cache.NodeSPI;
import org.jboss.cache.buddyreplication.BuddyFqnTransformer;
import org.jboss.cache.buddyreplication.BuddyManager;
@@ -171,9 +172,18 @@
else
{
// make sure we LOAD data for this node!!
- actualNode.getData();
- // and children!
- actualNode.getChildrenNames();
+ try
+ {
+ actualNode.getData();
+ // and children!
+ actualNode.getChildrenNames();
+ }
+ catch (NodeNotValidException e)
+ {
+ if (trace)
+ log.trace("Found node " + actualNode.getFqn() + " but
it is not valid. Returning 'no data found'", e);
+ return GravitateResult.noDataFound();
+ }
}
if (backupNodeFqn == null && searchSubtrees)
--
Brian Stansberry
Lead, AS Clustering
JBoss by Red Hat
11:18 a.m.
Thanks for the patch; that makes a lot of sense. Go ahead and check
it in to trunk.
Regarding the creation of nodes for deletion, well that's pessimistic
locking for you. Unfortunately the lock is encapsulated in the node
so you can't acquire the lock until the node is created. (Which is
partially why the pessimistic lock design sucks).
This is created in PessimisticNodeBasedLockManager (line 145). At
this stage we already know if we are just creating a dummy node for
deletion, so perhaps we could pass this as a flag into the constructor
of a PessimisticUnversionedNode, and this could acquire the lock on
creation. Hmm, need to think about this.
Cheers
Manik
On 6 Aug 2009, at 18:03, Brian Stansberry wrote:
More fun with buddy replication. :-)
Saw an error on 1 of our failover tests where,
1) Node D had left the group, so lots of gravitation was going on.
2) Various nodes were sending DataGravitationCleanupCommands to the
cluster for /BUDDY_BACKUP/D_DEAD/1/JSESSION/st_localhost/xxx. Result
is
all nodes in the cluster are trying to remove various
/BUDDY_BACKUP/D_DEAD/1/JSESSION/st_localhost/xxx nodes. On node A
those
nodes don't exist, so PessimisticLockInterceptor.handleRemoveCommand
is
adding them and removing them.
3) Concurrent with #2, a GravitateDataCommand for
/JSESSION/st_localhost/123 comes in to node A. Session 123 was never
stored on node A, so this should result in a cache miss. But what
happened once was:
[JBoss] 16:46:52,961 TRACE
[org.jboss.cache.marshall.CommandAwareRpcDispatcher]
(Incoming-13,10.34.32.153:14736) Problems invoking command.
[JBoss] org.jboss.cache.NodeNotValidException: Node
/_BUDDY_BACKUP_/10.34.32.156_48822:DEAD/1/JSESSION/st_localhost/
UvzutZkoESBMRSnjv0eTRA__
is not valid. Perhaps it has been moved or removed.
[JBoss] at
org
.jboss
.cache
.invocation
.NodeInvocationDelegate.assertValid(NodeInvocationDelegate.java:527)
[JBoss] at
org
.jboss
.cache
.invocation
.NodeInvocationDelegate.getChildrenNames(NodeInvocationDelegate.java:
292)
[JBoss] at
org
.jboss
.cache
.commands
.read.GravitateDataCommand.perform(GravitateDataCommand.java:176)
...
It seems the command is seeing a non-existent node. Yep; looking at
the
logs it's clear the above GravitateDataCommand was executed
concurrently
with another DataGravitationCleanupCommand for the same session. (I
need
to investigate why that happened.)
Below is a possible patch to work around the issue. This points to a
more general locking problem though -- should these "phantom nodes"
created for removal be visible to other threads? Shouldn't there be
a WL
on them from the moment they are created until after they are removed?
Hehe, answered my own question by writing it. The node is created by
PessimisticNodeBasedLockManager and then locked. There's a gap in
between where another thread could get a ref to it.
Anyway, the patch:
### Eclipse Workspace Patch 1.0
#P jbosscache-core
Index: src/main/java/org/jboss/cache/commands/read/
GravitateDataCommand.java
===================================================================
---
src/main/java/org/jboss/cache/commands/read/GravitateDataCommand.java
(revision 8163)
+++
src/main/java/org/jboss/cache/commands/read/GravitateDataCommand.java
(working copy)
@@ -29,6 +29,7 @@
import org.jboss.cache.InternalNode;
import org.jboss.cache.InvocationContext;
import org.jboss.cache.Node;
+import org.jboss.cache.NodeNotValidException;
import org.jboss.cache.NodeSPI;
import org.jboss.cache.buddyreplication.BuddyFqnTransformer;
import org.jboss.cache.buddyreplication.BuddyManager;
@@ -171,9 +172,18 @@
else
{
// make sure we LOAD data for this node!!
- actualNode.getData();
- // and children!
- actualNode.getChildrenNames();
+ try
+ {
+ actualNode.getData();
+ // and children!
+ actualNode.getChildrenNames();
+ }
+ catch (NodeNotValidException e)
+ {
+ if (trace)
+ log.trace("Found node " + actualNode.getFqn() + "
but
it is not valid. Returning 'no data found'", e);
+ return GravitateResult.noDataFound();
+ }
}
if (backupNodeFqn == null && searchSubtrees)
--
Brian Stansberry
Lead, AS Clustering
JBoss by Red Hat
_______________________________________________
jbosscache-dev mailing list
jbosscache-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/jbosscache-dev
--
Manik Surtani
manik(a)jboss.org
Lead, Infinispan
Lead, JBoss Cache
http://www.infinispan.org
http://www.jbosscache.org
11:28 a.m.
On 7 Aug 2009, at 10:18, Manik Surtani wrote:
Thanks for the patch; that makes a lot of sense. Go ahead and check
it in to trunk.
Never mind, I've added this myself so that I can put trunk through a
test run and cut 3.2.0.BETA1.
Cheers
Manik
Regarding the creation of nodes for deletion, well that's pessimistic
locking for you. Unfortunately the lock is encapsulated in the node
so you can't acquire the lock until the node is created. (Which is
partially why the pessimistic lock design sucks).
This is created in PessimisticNodeBasedLockManager (line 145). At
this stage we already know if we are just creating a dummy node for
deletion, so perhaps we could pass this as a flag into the constructor
of a PessimisticUnversionedNode, and this could acquire the lock on
creation. Hmm, need to think about this.
Cheers
Manik
On 6 Aug 2009, at 18:03, Brian Stansberry wrote:
> More fun with buddy replication. :-)
>
> Saw an error on 1 of our failover tests where,
>
> 1) Node D had left the group, so lots of gravitation was going on.
> 2) Various nodes were sending DataGravitationCleanupCommands to the
> cluster for /BUDDY_BACKUP/D_DEAD/1/JSESSION/st_localhost/xxx. Result
> is
> all nodes in the cluster are trying to remove various
> /BUDDY_BACKUP/D_DEAD/1/JSESSION/st_localhost/xxx nodes. On node A
> those
> nodes don't exist, so PessimisticLockInterceptor.handleRemoveCommand
> is
> adding them and removing them.
> 3) Concurrent with #2, a GravitateDataCommand for
> /JSESSION/st_localhost/123 comes in to node A. Session 123 was never
> stored on node A, so this should result in a cache miss. But what
> happened once was:
>
> [JBoss] 16:46:52,961 TRACE
> [org.jboss.cache.marshall.CommandAwareRpcDispatcher]
> (Incoming-13,10.34.32.153:14736) Problems invoking command.
> [JBoss] org.jboss.cache.NodeNotValidException: Node
> /_BUDDY_BACKUP_/10.34.32.156_48822:DEAD/1/JSESSION/st_localhost/
> UvzutZkoESBMRSnjv0eTRA__
> is not valid. Perhaps it has been moved or removed.
> [JBoss] at
> org
> .jboss
> .cache
> .invocation
> .NodeInvocationDelegate.assertValid(NodeInvocationDelegate.java:527)
> [JBoss] at
> org
> .jboss
> .cache
> .invocation
> .NodeInvocationDelegate.getChildrenNames(NodeInvocationDelegate.java:
> 292)
> [JBoss] at
> org
> .jboss
> .cache
> .commands
> .read.GravitateDataCommand.perform(GravitateDataCommand.java:176)
> ...
>
> It seems the command is seeing a non-existent node. Yep; looking at
> the
> logs it's clear the above GravitateDataCommand was executed
> concurrently
> with another DataGravitationCleanupCommand for the same session. (I
> need
> to investigate why that happened.)
>
> Below is a possible patch to work around the issue. This points to a
> more general locking problem though -- should these "phantom nodes"
> created for removal be visible to other threads? Shouldn't there be
> a WL
> on them from the moment they are created until after they are
> removed?
>
> Hehe, answered my own question by writing it. The node is created by
> PessimisticNodeBasedLockManager and then locked. There's a gap in
> between where another thread could get a ref to it.
>
> Anyway, the patch:
>
> ### Eclipse Workspace Patch 1.0
> #P jbosscache-core
> Index: src/main/java/org/jboss/cache/commands/read/
> GravitateDataCommand.java
> ===================================================================
> ---
> src/main/java/org/jboss/cache/commands/read/GravitateDataCommand.java
> (revision 8163)
> +++
> src/main/java/org/jboss/cache/commands/read/GravitateDataCommand.java
> (working copy)
> @@ -29,6 +29,7 @@
> import org.jboss.cache.InternalNode;
> import org.jboss.cache.InvocationContext;
> import org.jboss.cache.Node;
> +import org.jboss.cache.NodeNotValidException;
> import org.jboss.cache.NodeSPI;
> import org.jboss.cache.buddyreplication.BuddyFqnTransformer;
> import org.jboss.cache.buddyreplication.BuddyManager;
> @@ -171,9 +172,18 @@
> else
> {
> // make sure we LOAD data for this node!!
> - actualNode.getData();
> - // and children!
> - actualNode.getChildrenNames();
> + try
> + {
> + actualNode.getData();
> + // and children!
> + actualNode.getChildrenNames();
> + }
> + catch (NodeNotValidException e)
> + {
> + if (trace)
> + log.trace("Found node " + actualNode.getFqn() + "
> but
> it is not valid. Returning 'no data found'", e);
> + return GravitateResult.noDataFound();
> + }
> }
>
> if (backupNodeFqn == null && searchSubtrees)
>
>
> --
> Brian Stansberry
> Lead, AS Clustering
> JBoss by Red Hat
> _______________________________________________
> jbosscache-dev mailing list
> jbosscache-dev(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/jbosscache-dev
--
Manik Surtani
manik(a)jboss.org
Lead, Infinispan
Lead, JBoss Cache
http://www.infinispan.org
http://www.jbosscache.org
_______________________________________________
jbosscache-dev mailing list
jbosscache-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/jbosscache-dev
--
Manik Surtani
manik(a)jboss.org
Lead, Infinispan
Lead, JBoss Cache
http://www.infinispan.org
http://www.jbosscache.org
1 p.m.
On 7 Aug 2009, at 10:18, Manik Surtani wrote:
Thanks for the patch; that makes a lot of sense. Go ahead and check
it in to trunk.
Regarding the creation of nodes for deletion, well that's pessimistic
locking for you. Unfortunately the lock is encapsulated in the node
so you can't acquire the lock until the node is created. (Which is
partially why the pessimistic lock design sucks).
This is created in PessimisticNodeBasedLockManager (line 145). At
this stage we already know if we are just creating a dummy node for
deletion, so perhaps we could pass this as a flag into the constructor
of a PessimisticUnversionedNode, and this could acquire the lock on
creation. Hmm, need to think about this.
Right, so I have created a patch for this - it isn't the prettiest in
that it exposes some PessimisticUnversionedNode internals to the
PessimisticNodeBasedLockManager, but these two (deprecated) classes
are so tightly coupled anyway that if this fixes the race then I'm
happy to go with it.
Just as an FYI, this is checked in as rev. 8167.
Cheers
--
Manik Surtani
manik(a)jboss.org
Lead, Infinispan
Lead, JBoss Cache
http://www.infinispan.org
http://www.jbosscache.org
4:23 p.m.
Manik Surtani wrote:
On 7 Aug 2009, at 10:18, Manik Surtani wrote:
> Thanks for the patch; that makes a lot of sense. Go ahead and check
> it in to trunk.
>
> Regarding the creation of nodes for deletion, well that's pessimistic
> locking for you. Unfortunately the lock is encapsulated in the node
> so you can't acquire the lock until the node is created. (Which is
> partially why the pessimistic lock design sucks).
>
> This is created in PessimisticNodeBasedLockManager (line 145). At
> this stage we already know if we are just creating a dummy node for
> deletion, so perhaps we could pass this as a flag into the constructor
> of a PessimisticUnversionedNode, and this could acquire the lock on
> creation. Hmm, need to think about this.
Right, so I have created a patch for this - it isn't the prettiest in
that it exposes some PessimisticUnversionedNode internals to the
PessimisticNodeBasedLockManager, but these two (deprecated) classes are
so tightly coupled anyway that if this fixes the race then I'm happy to
go with it.
Just as an FYI, this is checked in as rev. 8167.
Great; thanks! Rado is doing some reconfig of his test setup; once
that's done he'll run the tests with a build of your latest.
--
Brian Stansberry
Lead, AS Clustering
JBoss by Red Hat
5:20 p.m.
On 7 Aug 2009, at 15:23, Brian Stansberry wrote:
Manik Surtani wrote:
> On 7 Aug 2009, at 10:18, Manik Surtani wrote:
>> Thanks for the patch; that makes a lot of sense. Go ahead and check
>> it in to trunk.
>>
>> Regarding the creation of nodes for deletion, well that's
>> pessimistic
>> locking for you. Unfortunately the lock is encapsulated in the node
>> so you can't acquire the lock until the node is created. (Which is
>> partially why the pessimistic lock design sucks).
>>
>> This is created in PessimisticNodeBasedLockManager (line 145). At
>> this stage we already know if we are just creating a dummy node for
>> deletion, so perhaps we could pass this as a flag into the
>> constructor
>> of a PessimisticUnversionedNode, and this could acquire the lock on
>> creation. Hmm, need to think about this.
> Right, so I have created a patch for this - it isn't the prettiest
> in that it exposes some PessimisticUnversionedNode internals to the
> PessimisticNodeBasedLockManager, but these two (deprecated) classes
> are so tightly coupled anyway that if this fixes the race then I'm
> happy to go with it.
> Just as an FYI, this is checked in as rev. 8167.
Great; thanks! Rado is doing some reconfig of his test setup; once
that's done he'll run the tests with a build of your latest.
Rado, you might want to do a run with this tag:
https://svn.jboss.org/repos/jbosscache/core/tags/3.2.0.BETA1/
--
Manik Surtani
manik(a)jboss.org
Lead, Infinispan
Lead, JBoss Cache
http://www.infinispan.org
http://www.jbosscache.org
6 p.m.
Great! I will build that one. Cheers
On 08/07/2009 05:20 PM, Manik Surtani wrote:
On 7 Aug 2009, at 15:23, Brian Stansberry wrote:
> Manik Surtani wrote:
>> On 7 Aug 2009, at 10:18, Manik Surtani wrote:
>>> Thanks for the patch; that makes a lot of sense. Go ahead and check
>>> it in to trunk.
>>>
>>> Regarding the creation of nodes for deletion, well that's pessimistic
>>> locking for you. Unfortunately the lock is encapsulated in the node
>>> so you can't acquire the lock until the node is created. (Which is
>>> partially why the pessimistic lock design sucks).
>>>
>>> This is created in PessimisticNodeBasedLockManager (line 145). At
>>> this stage we already know if we are just creating a dummy node for
>>> deletion, so perhaps we could pass this as a flag into the constructor
>>> of a PessimisticUnversionedNode, and this could acquire the lock on
>>> creation. Hmm, need to think about this.
>> Right, so I have created a patch for this - it isn't the prettiest in
>> that it exposes some PessimisticUnversionedNode internals to the
>> PessimisticNodeBasedLockManager, but these two (deprecated) classes
>> are so tightly coupled anyway that if this fixes the race then I'm
>> happy to go with it.
>> Just as an FYI, this is checked in as rev. 8167.
>
> Great; thanks! Rado is doing some reconfig of his test setup; once
> that's done he'll run the tests with a build of your latest.
Rado, you might want to do a run with this tag:
https://svn.jboss.org/repos/jbosscache/core/tags/3.2.0.BETA1/
--
Manik Surtani
manik(a)jboss.org
Lead, Infinispan
Lead, JBoss Cache
http://www.infinispan.org
http://www.jbosscache.org
--
Radoslav Husar
JBoss QE
Czech Republic
Mobile phone: +420 608 507 607
Office phone: +420 532 294 132; ext 82-62 132
http://intranet.corp.redhat.com/ic/intranet/RadoslavHusar.html
6:05 p.m.
Its in Maven as well
On 7 Aug 2009, at 17:00, Radoslav Husar wrote:
Great! I will build that one. Cheers
On 08/07/2009 05:20 PM, Manik Surtani wrote:
>
> On 7 Aug 2009, at 15:23, Brian Stansberry wrote:
>
>> Manik Surtani wrote:
>>> On 7 Aug 2009, at 10:18, Manik Surtani wrote:
>>>> Thanks for the patch; that makes a lot of sense. Go ahead and
>>>> check
>>>> it in to trunk.
>>>>
>>>> Regarding the creation of nodes for deletion, well that's
>>>> pessimistic
>>>> locking for you. Unfortunately the lock is encapsulated in the
>>>> node
>>>> so you can't acquire the lock until the node is created. (Which is
>>>> partially why the pessimistic lock design sucks).
>>>>
>>>> This is created in PessimisticNodeBasedLockManager (line 145). At
>>>> this stage we already know if we are just creating a dummy node
>>>> for
>>>> deletion, so perhaps we could pass this as a flag into the
>>>> constructor
>>>> of a PessimisticUnversionedNode, and this could acquire the lock
>>>> on
>>>> creation. Hmm, need to think about this.
>>> Right, so I have created a patch for this - it isn't the
>>> prettiest in
>>> that it exposes some PessimisticUnversionedNode internals to the
>>> PessimisticNodeBasedLockManager, but these two (deprecated) classes
>>> are so tightly coupled anyway that if this fixes the race then I'm
>>> happy to go with it.
>>> Just as an FYI, this is checked in as rev. 8167.
>>
>> Great; thanks! Rado is doing some reconfig of his test setup; once
>> that's done he'll run the tests with a build of your latest.
>
> Rado, you might want to do a run with this tag:
>
> https://svn.jboss.org/repos/jbosscache/core/tags/3.2.0.BETA1/
>
>
> --
> Manik Surtani
> manik(a)jboss.org
> Lead, Infinispan
> Lead, JBoss Cache
> http://www.infinispan.org
> http://www.jbosscache.org
>
>
>
>
--
Radoslav Husar
JBoss QE
Czech Republic
Mobile phone: +420 608 507 607
Office phone: +420 532 294 132; ext 82-62 132
http://intranet.corp.redhat.com/ic/intranet/RadoslavHusar.html
--
Manik Surtani
manik(a)jboss.org
Lead, Infinispan
Lead, JBoss Cache
http://www.infinispan.org
http://www.jbosscache.org
5369
days inactive
5370
days old
jbosscache-dev@lists.jboss.org
7 comments
3 participants
participants (3)
-
Brian Stansberry -
Manik Surtani -
Radoslav Husar