You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Carsten Ziegeler <cz...@apache.org> on 2010/04/14 14:47:53 UTC
Re: svn commit: r933747 - in /sling/trunk/bundles/jcr/resource: ./
src/main/java/org/apache/sling/jcr/resource/ src/main/java/org/apache/sling/jcr/resource/internal/
src/main/java/org/apache/sling/jcr/resource/internal/helper/ src/test/java/org/apache/slin...
Hi Justin,
thanks for the changes as we have discussed them roughly. I'm now more
happy with the way of handling this :)
It seems that these changes broke the tests for the jcr resource bundle
- at least on my machine it is failing as the used second workspace is
not available.
I've just committed a change in revision 933928. While I tested your
changes I noticed a drop in performance of nearly 100% with every
request (in our environment which does literally a lot during a
request). Now, it seems that the check for the new path syntax
[workspace:path] causes trouble in our environment as we have paths with
a colon somewhere in the middle.
So I went ahead and changed the check from ":" to ":/" and changed also
the regex based split to a string substring operation.
This brought us nearly back to the performace we had before - I
currently note a drop of roughly 10% though I'm not 100% sure that these
are caused by these changes. I'll further investigate.
Anyway, to cut a long story short :) it seems that my initial idea of
just using the ":" as a separator might cause trouble. One solution is
to go like I did with the quick changes and require a path to be
absolute if the workspace prefixed notation is used.
Or we are searching for a different character to separate the two parts?
In addition, I'm wondering if we could make this feature completly
optional (maybe by a resource resolver wrapper etc.)? (But I think this
is something we can look into later on)
Regards
Carsten
justin wrote
> Author: justin
> Date: Tue Apr 13 19:13:31 2010
> New Revision: 933747
>
> URL: http://svn.apache.org/viewvc?rev=933747&view=rev
> Log:
> SLING-1447 - committing first pass at workspace names in resource paths
>
> Added:
> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/WorkspaceDecoratedResource.java
> sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrResourceListenerTest.java
> sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/SynchronousJcrResourceListener.java
> Modified:
> sling/trunk/bundles/jcr/resource/pom.xml
> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrResourceConstants.java
> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/JcrResourceListener.java
> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/JcrResourceResolver.java
> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/JcrResourceResolverFactoryImpl.java
> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/ResourceDecoratorTracker.java
> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/ResourceIteratorDecorator.java
> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/helper/ResourceProviderEntry.java
> sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrResourceResolverTest.java
--
Carsten Ziegeler
cziegeler@apache.org
Re: svn commit: r933747 - in /sling/trunk/bundles/jcr/resource: ./
src/main/java/org/apache/sling/jcr/resource/ src/main/java/org/apache/sling/jcr/resource/internal/
src/main/java/org/apache/sling/jcr/resource/internal/helper/ src/test/java/org/apache/slin...
Posted by Justin Edelson <ju...@gmail.com>.
On 4/14/10 2:22 PM, Carsten Ziegeler wrote:
> Carsten Ziegeler wrote
>> Justin Edelson wrote
>
>>> I think this should just be a config option on
>>> JcrResourceResolverFactoryImpl which bypasses the new code blocks
>>> entirely. The only thing we need to watch out for here is that if you
>>> enable non-default workspace observation, non-default workspace
>>> resolution must be enabled, but this is reasonably simple to deal with.
>> Ok, right - I think the configuration part is no problem.
>> I'll try to add a switch to the code and see how it turns out
>>
> WDYT about using the already available
> "resource.resolver.listener.workspaces" property from the factory?
>
> If this is set we enable the code, if this is not set, we disable the
> code. Or is it more logical to have a separate boolean option?
Maybe the other way around - replace this property with
"resource.resolver.multiworkspace" which, if true, attaches listeners to
all workspaces. There may be no need to listen to only a subset of
workspaces.
Justin
>
> Regards
> Carsten
>
Re: svn commit: r933747 - in /sling/trunk/bundles/jcr/resource: ./
src/main/java/org/apache/sling/jcr/resource/ src/main/java/org/apache/sling/jcr/resource/internal/
src/main/java/org/apache/sling/jcr/resource/internal/helper/ src/test/java/org/apache/slin...
Posted by Carsten Ziegeler <cz...@apache.org>.
Carsten Ziegeler wrote
> Justin Edelson wrote
>> I think this should just be a config option on
>> JcrResourceResolverFactoryImpl which bypasses the new code blocks
>> entirely. The only thing we need to watch out for here is that if you
>> enable non-default workspace observation, non-default workspace
>> resolution must be enabled, but this is reasonably simple to deal with.
> Ok, right - I think the configuration part is no problem.
> I'll try to add a switch to the code and see how it turns out
>
WDYT about using the already available
"resource.resolver.listener.workspaces" property from the factory?
If this is set we enable the code, if this is not set, we disable the
code. Or is it more logical to have a separate boolean option?
Regards
Carsten
--
Carsten Ziegeler
cziegeler@apache.org
Re: svn commit: r933747 - in /sling/trunk/bundles/jcr/resource: ./
src/main/java/org/apache/sling/jcr/resource/ src/main/java/org/apache/sling/jcr/resource/internal/
src/main/java/org/apache/sling/jcr/resource/internal/helper/ src/test/java/org/apache/slin...
Posted by Carsten Ziegeler <cz...@apache.org>.
Justin Edelson wrote
> Argh. I see the problem now. I had setup code to create the ws2
> workspace in the Listener test, but not in the Resolver test. Fixed in
> r933956.
Cool, thanks!
> Makes sense. This code is not optimized, but I wanted to get something
> committed.
Yes, sure - no problem - that's the way open source works :)
I was just in the middle of performance tests and got it by the changes,
but that's more my fault :)
> Ideally, there's a character which is:
> 1) illegal in a JCR node name
> 2) legal in a URL path
>
> I just don't know off the top of my head what that would be.
Yepp - I don't have a clue either.
>
> Barring this, I agree that requiring an absolute path is the right
> solution. If you need to pass a relative path to getResource(), you
> could always specify a search path entry like "ws2:/" in the
> JcrResourceResolverFactoryImpl config. Or use the two-argument form of
> getResource().
Ok, so I guess we can go with the current solution.
>> In addition, I'm wondering if we could make this feature completly
>> optional (maybe by a resource resolver wrapper etc.)? (But I think this
>> is something we can look into later on)
> I think this should just be a config option on
> JcrResourceResolverFactoryImpl which bypasses the new code blocks
> entirely. The only thing we need to watch out for here is that if you
> enable non-default workspace observation, non-default workspace
> resolution must be enabled, but this is reasonably simple to deal with.
Ok, right - I think the configuration part is no problem.
I'll try to add a switch to the code and see how it turns out
Regards
Carsten
>
> Justin
>
>>
>> Regards
>> Carsten
>>
>> justin wrote
>>> Author: justin
>>> Date: Tue Apr 13 19:13:31 2010
>>> New Revision: 933747
>>>
>>> URL: http://svn.apache.org/viewvc?rev=933747&view=rev
>>> Log:
>>> SLING-1447 - committing first pass at workspace names in resource paths
>>>
>>> Added:
>>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/WorkspaceDecoratedResource.java
>>> sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrResourceListenerTest.java
>>> sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/SynchronousJcrResourceListener.java
>>> Modified:
>>> sling/trunk/bundles/jcr/resource/pom.xml
>>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrResourceConstants.java
>>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/JcrResourceListener.java
>>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/JcrResourceResolver.java
>>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/JcrResourceResolverFactoryImpl.java
>>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/ResourceDecoratorTracker.java
>>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/ResourceIteratorDecorator.java
>>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/helper/ResourceProviderEntry.java
>>> sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrResourceResolverTest.java
>>
>
>
--
Carsten Ziegeler
cziegeler@apache.org
Re: svn commit: r933747 - in /sling/trunk/bundles/jcr/resource: ./
src/main/java/org/apache/sling/jcr/resource/ src/main/java/org/apache/sling/jcr/resource/internal/
src/main/java/org/apache/sling/jcr/resource/internal/helper/ src/test/java/org/apache/slin...
Posted by Justin Edelson <ju...@gmail.com>.
On 4/14/10 8:47 AM, Carsten Ziegeler wrote:
> Hi Justin,
>
> thanks for the changes as we have discussed them roughly. I'm now more
> happy with the way of handling this :)
Well, that's a start...
>
> It seems that these changes broke the tests for the jcr resource bundle
> - at least on my machine it is failing as the used second workspace is
> not available.
Argh. I see the problem now. I had setup code to create the ws2
workspace in the Listener test, but not in the Resolver test. Fixed in
r933956.
>
> I've just committed a change in revision 933928. While I tested your
> changes I noticed a drop in performance of nearly 100% with every
> request (in our environment which does literally a lot during a
> request). Now, it seems that the check for the new path syntax
> [workspace:path] causes trouble in our environment as we have paths with
> a colon somewhere in the middle.
> So I went ahead and changed the check from ":" to ":/" and changed also
> the regex based split to a string substring operation.
> This brought us nearly back to the performace we had before - I
> currently note a drop of roughly 10% though I'm not 100% sure that these
> are caused by these changes. I'll further investigate.
Makes sense. This code is not optimized, but I wanted to get something
committed.
>
> Anyway, to cut a long story short :) it seems that my initial idea of
> just using the ":" as a separator might cause trouble. One solution is
> to go like I did with the quick changes and require a path to be
> absolute if the workspace prefixed notation is used.
> Or we are searching for a different character to separate the two parts?
Ideally, there's a character which is:
1) illegal in a JCR node name
2) legal in a URL path
I just don't know off the top of my head what that would be.
Barring this, I agree that requiring an absolute path is the right
solution. If you need to pass a relative path to getResource(), you
could always specify a search path entry like "ws2:/" in the
JcrResourceResolverFactoryImpl config. Or use the two-argument form of
getResource().
>
> In addition, I'm wondering if we could make this feature completly
> optional (maybe by a resource resolver wrapper etc.)? (But I think this
> is something we can look into later on)
I think this should just be a config option on
JcrResourceResolverFactoryImpl which bypasses the new code blocks
entirely. The only thing we need to watch out for here is that if you
enable non-default workspace observation, non-default workspace
resolution must be enabled, but this is reasonably simple to deal with.
Justin
>
> Regards
> Carsten
>
> justin wrote
>> Author: justin
>> Date: Tue Apr 13 19:13:31 2010
>> New Revision: 933747
>>
>> URL: http://svn.apache.org/viewvc?rev=933747&view=rev
>> Log:
>> SLING-1447 - committing first pass at workspace names in resource paths
>>
>> Added:
>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/WorkspaceDecoratedResource.java
>> sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrResourceListenerTest.java
>> sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/SynchronousJcrResourceListener.java
>> Modified:
>> sling/trunk/bundles/jcr/resource/pom.xml
>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/JcrResourceConstants.java
>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/JcrResourceListener.java
>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/JcrResourceResolver.java
>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/JcrResourceResolverFactoryImpl.java
>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/ResourceDecoratorTracker.java
>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/ResourceIteratorDecorator.java
>> sling/trunk/bundles/jcr/resource/src/main/java/org/apache/sling/jcr/resource/internal/helper/ResourceProviderEntry.java
>> sling/trunk/bundles/jcr/resource/src/test/java/org/apache/sling/jcr/resource/internal/JcrResourceResolverTest.java
>