Re: Reloading node types in clustered environment

[Berry van Halderen <b....@1hippo.com>]

On Tue, Dec 6, 2011 at 4:40 PM, Berry van Halderen
<b....@1hippo.com> wrote:
> We've run into log messages that have come us to suspect one of our
> practices, namely to reregister node types.  The reregistering occurs
> in a clustered environment, where other clustered nodes keep on using
> the node types, but on rare occasion certain exceptions are thrown,
> most particular an exception from getEffetiveNodeType when retrieving
> a node from the persistent store.  I've still to evaluate if it is
> indeed the case, but we're wondering if it is at all safe to do
> changing node type operations in a clustered environment.
> Note, before 2.2.x we did not see these kind of errors, currently on
> 2.2.9 we are.

I was able myself to track it down.  It turns out that before
re-registering there
was also a call to unregister a node type. That caused a small gap
between the unregister and the re-register where other cluster nodes could
have received the unregister but not the new type.

I've fixed this in our own code, but still I'm a bit worried about this change
of behavior It used to be impossible to unregister a node type when it
was still in use (or at least never to unregister a node type).  Not being
able to unregister a node type when still in use seems to be the most
correct semantics.

But what actually happened in Jackrabbit was actually quite scary.
By being able to unregister a node type in use, other nodes load and
modify a node with a mixin type that is being unregistered.  The
mixin type is silently dropped from the node, but any properties tied
to the mixin node type are kept.  There is a window where such a
node can be persisted back into database.

What is then left in the persisted storage is a node with properties
that cannot be part of that node according to its node type.  And
this node cannot be loaded, cannot be deleted, cannot be fixed.
The only option is to go into sql level to delete the node or fix the
binary bundle data stored in the record.

Maybe it's a good idea that we're not able to remove node types that
are still in use, wdyt?

\Berry

Re: Reloading node types in clustered environment

[Berry van Halderen <b....@1hippo.com>]

On Fri, Jan 27, 2012 at 3:29 PM, Jukka Zitting <ju...@gmail.com> wrote:
> Please do so. I'm all set to cut the 2.2.11 release candidate, and
> having this fix in would be great.

Go ahead, it's committed.

Thx,
\Berry

Re: Reloading node types in clustered environment

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Fri, Jan 27, 2012 at 2:26 PM, Berry van Halderen
<b....@1hippo.com> wrote:
> Small investigation shows that some fix for JCR-2587 inadvertently
> contained this change, which was reverted (much) later on the trunk,
> but after the 2.1.x and 2.2.x branch were made.

Argh, we definitely should fix that. Good catch!

> Though not a big issue, I'd like to merge this change also back
> into the 2.1.x and 2.2.x branches (unless objections, I'll do this).

Please do so. I'm all set to cut the 2.2.11 release candidate, and
having this fix in would be great.

BR,

Jukka Zitting

Re: Reloading node types in clustered environment

[Berry van Halderen <b....@1hippo.com>]

On Fri, Jan 27, 2012 at 11:30 AM, Jukka Zitting <ju...@gmail.com> wrote:
> On Fri, Jan 27, 2012 at 10:35 AM, Berry van Halderen
> <b....@1hippo.com> wrote:
> That should still be the case. The
> NodeTypeRegistry.unregisterNodeTypes() method calls the protected
> checkForReferencesInContent() method before actually removing the
> type. The default implementation of checkForReferencesInContent()
> simply throws a RepositoryException with a "not yet implemented"
> message, which in practice prevents any node types from being
> unregisted.

Which is the problem actually, the throw statement in
checkForReferencesInContent() is commented out in the 2.2.x versions.
Small investigation shows that some fix for JCR-2587 inadvertently
contained this change, which was reverted (much) later on the trunk,
but after the 2.1.x and 2.2.x branch were made.
Hence this will work on trunk and 2.3.x, but not on 2.1.x and 2.2.x
branches.
Though not a big issue, I'd like to merge this change also back
into the 2.1.x and 2.2.x branches (unless objections, I'll do this).

\Berry

Re: Reloading node types in clustered environment

[Stefan Guggisberg <st...@gmail.com>]

On Fri, Jan 27, 2012 at 11:30 AM, Jukka Zitting <ju...@gmail.com> wrote:
> Hi,
>
> On Fri, Jan 27, 2012 at 10:35 AM, Berry van Halderen
> <b....@1hippo.com> wrote:
>> I've fixed this in our own code, but still I'm a bit worried about this change
>> of behavior It used to be impossible to unregister a node type when it
>> was still in use (or at least never to unregister a node type).  Not being
>> able to unregister a node type when still in use seems to be the most
>> correct semantics.
>
> That should still be the case. The
> NodeTypeRegistry.unregisterNodeTypes() method calls the protected
> checkForReferencesInContent() method before actually removing the
> type. The default implementation of checkForReferencesInContent()
> simply throws a RepositoryException with a "not yet implemented"
> message, which in practice prevents any node types from being
> unregisted.
>
> Until a the checkForReferencesInContent() is properly implemented (see
> the javadoc for ideas on how to do that), it's possible for downstream
> projects or deployments to override the method in case they already
> have some other mechanism for guaranteeing that a node type can safely
> be removed. I suppose the repository you're using does override the
> method, but doesn't give such a strong guarantee of consistency.
>

doh! jukka beat me by a few seconds ;)

cheers
stefan

> BR,
>
> Jukka Zitting

Re: Reloading node types in clustered environment

[Jukka Zitting <ju...@gmail.com>]

Hi,

On Fri, Jan 27, 2012 at 10:35 AM, Berry van Halderen
<b....@1hippo.com> wrote:
> I've fixed this in our own code, but still I'm a bit worried about this change
> of behavior It used to be impossible to unregister a node type when it
> was still in use (or at least never to unregister a node type).  Not being
> able to unregister a node type when still in use seems to be the most
> correct semantics.

That should still be the case. The
NodeTypeRegistry.unregisterNodeTypes() method calls the protected
checkForReferencesInContent() method before actually removing the
type. The default implementation of checkForReferencesInContent()
simply throws a RepositoryException with a "not yet implemented"
message, which in practice prevents any node types from being
unregisted.

Until a the checkForReferencesInContent() is properly implemented (see
the javadoc for ideas on how to do that), it's possible for downstream
projects or deployments to override the method in case they already
have some other mechanism for guaranteeing that a node type can safely
be removed. I suppose the repository you're using does override the
method, but doesn't give such a strong guarantee of consistency.

BR,

Jukka Zitting

Re: Reloading node types in clustered environment

[Stefan Guggisberg <st...@gmail.com>]

On Fri, Jan 27, 2012 at 10:35 AM, Berry van Halderen
<b....@1hippo.com> wrote:
> On Tue, Dec 6, 2011 at 4:40 PM, Berry van Halderen
> <b....@1hippo.com> wrote:
>> We've run into log messages that have come us to suspect one of our
>> practices, namely to reregister node types.  The reregistering occurs
>> in a clustered environment, where other clustered nodes keep on using
>> the node types, but on rare occasion certain exceptions are thrown,
>> most particular an exception from getEffetiveNodeType when retrieving
>> a node from the persistent store.  I've still to evaluate if it is
>> indeed the case, but we're wondering if it is at all safe to do
>> changing node type operations in a clustered environment.
>> Note, before 2.2.x we did not see these kind of errors, currently on
>> 2.2.9 we are.
>
> I was able myself to track it down.  It turns out that before
> re-registering there
> was also a call to unregister a node type. That caused a small gap
> between the unregister and the re-register where other cluster nodes could
> have received the unregister but not the new type.
>
> I've fixed this in our own code, but still I'm a bit worried about this change
> of behavior It used to be impossible to unregister a node type when it
> was still in use (or at least never to unregister a node type).  Not being
> able to unregister a node type when still in use seems to be the most
> correct semantics.

agreed, but unortunately also the most expensive/difficult to implement...

AFAIK it's currently not possible to unregister a node type through the
public api (javax.jcr.NodeTypeManager#unregisterNodeType)

the key here is the following method:

NodeTypeRegistry.checkForReferencesInContent()

this method currently throws unconditionally. the javadoc explains
why.

>
> But what actually happened in Jackrabbit was actually quite scary.
> By being able to unregister a node type in use, other nodes load and
> modify a node with a mixin type that is being unregistered.  The
> mixin type is silently dropped from the node, but any properties tied
> to the mixin node type are kept.  There is a window where such a
> node can be persisted back into database.
>
> What is then left in the persisted storage is a node with properties
> that cannot be part of that node according to its node type.  And
> this node cannot be loaded, cannot be deleted, cannot be fixed.
> The only option is to go into sql level to delete the node or fix the
> binary bundle data stored in the record.
>
> Maybe it's a good idea that we're not able to remove node types that
> are still in use, wdyt?

theoretically, yes. the hard part is to determine whether it's
actually in use...

cheers
stefan
>
> \Berry