You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by st...@apache.org on 2012/04/04 17:36:49 UTC
svn commit: r1309458 -
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Author: stefan
Date: Wed Apr 4 15:36:49 2012
New Revision: 1309458
URL: http://svn.apache.org/viewvc?rev=1309458&view=rev
Log:
optimized getChildNode
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java?rev=1309458&r1=1309457&r2=1309458&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java Wed Apr 4 15:36:49 2012
@@ -20,7 +20,6 @@ package org.apache.jackrabbit.oak.kernel
import org.apache.jackrabbit.mk.model.Scalar;
import org.apache.jackrabbit.mk.api.MicroKernel;
-import org.apache.jackrabbit.mk.api.MicroKernelException;
import org.apache.jackrabbit.mk.json.JsopReader;
import org.apache.jackrabbit.mk.json.JsopTokenizer;
import org.apache.jackrabbit.mk.model.AbstractNodeState;
@@ -141,11 +140,8 @@ class KernelNodeState extends AbstractNo
NodeState child = childNodes.get(name);
if (child == null && childNodeCount > MAX_CHILD_NODE_NAMES) {
String childPath = getChildPath(name);
- try {
- kernel.getNodes(childPath, revision, 0, 0, 0, null);
+ if (kernel.nodeExists(childPath, revision)) {
child = new KernelNodeState(kernel, childPath, revision);
- } catch (MicroKernelException e) {
- // FIXME: Better way to determine whether a child node exists
}
}
return child;
Re: svn commit: r1309458 -
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
>Then the storage mechanism doesn't implement the MicroKernel contract
>as currently specified.
>
>If we want to allow such implementations (one related example was just
>discussed in relation to garbage collection), they need to be
>reflected in the MicroKernel contract. The later we do that, the more
>expensive it will be to adjust all the code above the MK layer. So if
>this is a genuine need, we should discuss it ASAP.
To be able to discuss it, I think we need a prototype for a scalable
MongoDB MicroKernel implementation. Or at least we need a good
documentation on how such an implementation would look like.
Regards,
Thomas
Re: svn commit: r1309458 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Posted by Jukka Zitting <ju...@gmail.com>.
HI,
On Thu, Apr 5, 2012 at 11:52 AM, Thomas Mueller <mu...@adobe.com> wrote:
> But what if the revision is no longer available, or if the storage
> mechanism doesn't support repeatable, consistent reading of all revisions
> (eventually consistent model or read committed transaction isolation and
> not MVCC).
Then the storage mechanism doesn't implement the MicroKernel contract
as currently specified.
If we want to allow such implementations (one related example was just
discussed in relation to garbage collection), they need to be
reflected in the MicroKernel contract. The later we do that, the more
expensive it will be to adjust all the code above the MK layer. So if
this is a genuine need, we should discuss it ASAP.
BR,
Jukka Zitting
Re: svn commit: r1309458 -
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
>Revisions are immutable, so this should never happen.
In theory, yes.
But what if the revision is no longer available, or if the storage
mechanism doesn't support repeatable, consistent reading of all revisions
(eventually consistent model or read committed transaction isolation and
not MVCC).
Regards,
Thomas
Re: svn commit: r1309458 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Thu, Apr 5, 2012 at 10:48 AM, Thomas Mueller <mu...@adobe.com> wrote:
> I think the pattern "if nodeExists then getNodes" should be avoided also
> because the node might have been deleted just after calling nodeExists,
> depending on the underlying storage.
Revisions are immutable, so this should never happen.
BR,
Jukka Zitting
Re: svn commit: r1309458 -
/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Posted by Thomas Mueller <mu...@adobe.com>.
Hi,
>>That's what I was wondering, is a nodeExists() call followed by
>> getNodes() better than a getNodes() call by itself? Especially if we
>> assume potential network overhead for both calls...
>
>That pattern pops up all over again. We should probably not use
>exceptions for signalling missing values but use null instead and use
>exceptions for exceptional cases.
I agree. What about
(a) getNodes() can return null, like java.util.Map.get(), or
(b) getNodesOrNull() is added, and getNodes would still throw an exception.
I think the pattern "if nodeExists then getNodes" should be avoided also
because the node might have been deleted just after calling nodeExists,
depending on the underlying storage.
Regards,
Thomas
Re: svn commit: r1309458 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Posted by Michael Dürig <md...@apache.org>.
On 5.4.12 8:00, Jukka Zitting wrote:
> Hi,
>
> On Wed, Apr 4, 2012 at 5:36 PM,<st...@apache.org> wrote:
>> optimized getChildNode
>> [...]
>> if (child == null&& childNodeCount> MAX_CHILD_NODE_NAMES) {
>> String childPath = getChildPath(name);
>> - try {
>> - kernel.getNodes(childPath, revision, 0, 0, 0, null);
>> + if (kernel.nodeExists(childPath, revision)) {
>> child = new KernelNodeState(kernel, childPath, revision);
>> - } catch (MicroKernelException e) {
>> - // FIXME: Better way to determine whether a child node exists
>> }
>> }
>
> That's what I was wondering, is a nodeExists() call followed by
> getNodes() better than a getNodes() call by itself? Especially if we
> assume potential network overhead for both calls...
That pattern pops up all over again. We should probably not use
exceptions for signalling missing values but use null instead and use
exceptions for exceptional cases.
Michael
>
> I'll spend some time over the next few days to get us started with
> initial performance benchmarks. That should help us make more informed
> decisions on cases like this.
>
> BR,
>
> Jukka Zitting
Re: svn commit: r1309458 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Thu, Apr 5, 2012 at 9:43 AM, Stefan Guggisberg
<st...@gmail.com> wrote:
> you're right, but in this particular case i replaced a dummy getNodes() call
> in a try/catch block with a single nodeExists() call returning a boolean.
> i'd argue that the code is cleaner and probably not slower ;)
Right, I was confusing this with an earlier KernelNodeState design
that repeated the getNodes() call in the constructor. Sorry for the
noise.
BR,
Jukka Zitting
Re: svn commit: r1309458 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Posted by Stefan Guggisberg <st...@gmail.com>.
On Thu, Apr 5, 2012 at 9:00 AM, Jukka Zitting <ju...@gmail.com> wrote:
> Hi,
>
> On Wed, Apr 4, 2012 at 5:36 PM, <st...@apache.org> wrote:
>> optimized getChildNode
>> [...]
>> if (child == null && childNodeCount > MAX_CHILD_NODE_NAMES) {
>> String childPath = getChildPath(name);
>> - try {
>> - kernel.getNodes(childPath, revision, 0, 0, 0, null);
>> + if (kernel.nodeExists(childPath, revision)) {
>> child = new KernelNodeState(kernel, childPath, revision);
>> - } catch (MicroKernelException e) {
>> - // FIXME: Better way to determine whether a child node exists
>> }
>> }
>
> That's what I was wondering, is a nodeExists() call followed by
> getNodes() better than a getNodes() call by itself? Especially if we
> assume potential network overhead for both calls...
you're right, but in this particular case i replaced a dummy getNodes() call
in a try/catch block with a single nodeExists() call returning a boolean.
i'd argue that the code is cleaner and probably not slower ;)
cheers
stefan
>
> I'll spend some time over the next few days to get us started with
> initial performance benchmarks. That should help us make more informed
> decisions on cases like this.
>
> BR,
>
> Jukka Zitting
Re: svn commit: r1309458 - /jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/KernelNodeState.java
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Apr 4, 2012 at 5:36 PM, <st...@apache.org> wrote:
> optimized getChildNode
> [...]
> if (child == null && childNodeCount > MAX_CHILD_NODE_NAMES) {
> String childPath = getChildPath(name);
> - try {
> - kernel.getNodes(childPath, revision, 0, 0, 0, null);
> + if (kernel.nodeExists(childPath, revision)) {
> child = new KernelNodeState(kernel, childPath, revision);
> - } catch (MicroKernelException e) {
> - // FIXME: Better way to determine whether a child node exists
> }
> }
That's what I was wondering, is a nodeExists() call followed by
getNodes() better than a getNodes() call by itself? Especially if we
assume potential network overhead for both calls...
I'll spend some time over the next few days to get us started with
initial performance benchmarks. That should help us make more informed
decisions on cases like this.
BR,
Jukka Zitting