Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2024-01-09 Thread via GitHub 


reschke merged PR #111:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2024-01-09 Thread via GitHub 


reschke commented on code in PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#discussion_r1445794252


##
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##
@@ -1148,19 +1148,26 @@ private boolean addEntry(final Map> entryMap, final Strin
  */
 private Map>> loadAliases(final 
ResourceResolver resolver) {
 final Map>> map = new 
ConcurrentHashMap<>();
-final String queryString = generateAliasQuery();
+final String baseQueryString = generateAliasQuery();
 
-log.debug("start alias query: {}", queryString);
-long queryStart = System.nanoTime();
-final Iterator i = resolver.findResources(queryString, 
"JCR-SQL2");
-long queryElapsed = System.nanoTime() - queryStart;
-log.debug("end alias query; elapsed {}ms", 
TimeUnit.NANOSECONDS.toMillis(queryElapsed));
+Iterator it;
+try {
+final String queryStringWithSort = baseQueryString + " AND 
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+it = new PagedQueryIterator("alias", "sling:alias", resolver, 
queryStringWithSort,
+Integer.getInteger("sling.alias.pageSize", 2000));

Review Comment:
   -> https://issues.apache.org/jira/browse/SLING-12225



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2024-01-08 Thread via GitHub 


bdelacretaz commented on PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#issuecomment-1881409583

   > ...the discussion about whether or not to use the system property in is 
... interesting, but blocks progress here...
   
   I don't think there are any objections left if that usage is clearly 
expressed in code. Combining my previous suggestion with the additional 
comments about it, I think defining the page size as below, instead of inline 
in the code, would work for all involved in this discussion:
   
   // Using a system property here, as changing this value only makes sense
   // for debugging and code performance testing
   private static int ALIAS_PAGE_SIZE = 
Integer.getInteger("sling.debug.alias.pageSize", 2000));


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2024-01-08 Thread via GitHub 


reschke commented on PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#issuecomment-1881240896

   FWIW, the discussion about whether or not to use the system property in is 
... interesting, but blocks progress here. IMHO we should do that as part of a 
separate ticket, which would address the same question for sling:vanityPath as 
well. For sure we want consistency here, no?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2024-01-08 Thread via GitHub 


reschke commented on code in PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#discussion_r1444837180


##
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##
@@ -1148,19 +1148,26 @@ private boolean addEntry(final Map> entryMap, final Strin
  */
 private Map>> loadAliases(final 
ResourceResolver resolver) {
 final Map>> map = new 
ConcurrentHashMap<>();
-final String queryString = generateAliasQuery();
+final String baseQueryString = generateAliasQuery();
 
-log.debug("start alias query: {}", queryString);
-long queryStart = System.nanoTime();
-final Iterator i = resolver.findResources(queryString, 
"JCR-SQL2");
-long queryElapsed = System.nanoTime() - queryStart;
-log.debug("end alias query; elapsed {}ms", 
TimeUnit.NANOSECONDS.toMillis(queryElapsed));
+Iterator it;
+try {
+final String queryStringWithSort = baseQueryString + " AND 
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+it = new PagedQueryIterator("alias", "sling:alias", resolver, 
queryStringWithSort,
+Integer.getInteger("sling.alias.pageSize", 2000));

Review Comment:
   Maybe replace "unsupported" by "debug"?
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2024-01-08 Thread via GitHub 


rombert commented on code in PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#discussion_r112059


##
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##
@@ -1148,19 +1148,26 @@ private boolean addEntry(final Map> entryMap, final Strin
  */
 private Map>> loadAliases(final 
ResourceResolver resolver) {
 final Map>> map = new 
ConcurrentHashMap<>();
-final String queryString = generateAliasQuery();
+final String baseQueryString = generateAliasQuery();
 
-log.debug("start alias query: {}", queryString);
-long queryStart = System.nanoTime();
-final Iterator i = resolver.findResources(queryString, 
"JCR-SQL2");
-long queryElapsed = System.nanoTime() - queryStart;
-log.debug("end alias query; elapsed {}ms", 
TimeUnit.NANOSECONDS.toMillis(queryElapsed));
+Iterator it;
+try {
+final String queryStringWithSort = baseQueryString + " AND 
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+it = new PagedQueryIterator("alias", "sling:alias", resolver, 
queryStringWithSort,
+Integer.getInteger("sling.alias.pageSize", 2000));

Review Comment:
   Looking at the code is not enough to understand that this is intended for 
internal experiments only, which explains the "big" conversation I started 
because of it. I like @bdelacretaz's suggestion on how to extract this, and 
would add the suggestion to name the property 
"sling.unsupported.alias.pageSize", for clarity.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2024-01-08 Thread via GitHub 


bdelacretaz commented on code in PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#discussion_r1444395825


##
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##
@@ -1148,19 +1148,26 @@ private boolean addEntry(final Map> entryMap, final Strin
  */
 private Map>> loadAliases(final 
ResourceResolver resolver) {
 final Map>> map = new 
ConcurrentHashMap<>();
-final String queryString = generateAliasQuery();
+final String baseQueryString = generateAliasQuery();
 
-log.debug("start alias query: {}", queryString);
-long queryStart = System.nanoTime();
-final Iterator i = resolver.findResources(queryString, 
"JCR-SQL2");
-long queryElapsed = System.nanoTime() - queryStart;
-log.debug("end alias query; elapsed {}ms", 
TimeUnit.NANOSECONDS.toMillis(queryElapsed));
+Iterator it;
+try {
+final String queryStringWithSort = baseQueryString + " AND 
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+it = new PagedQueryIterator("alias", "sling:alias", resolver, 
queryStringWithSort,
+Integer.getInteger("sling.alias.pageSize", 2000));

Review Comment:
   If there's agreement on using the system property for experimenting only, I 
think defining it as below would clarify that:
   
   // Using a system property here, as changing this value only makes sense
   // for debugging and code performance testing
   private static int ALIAS_PAGE_SIZE = 
Integer.getInteger("sling.alias.pageSize", 2000));



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2024-01-05 Thread via GitHub 


joerghoh commented on code in PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#discussion_r1443021563


##
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##
@@ -1148,19 +1148,26 @@ private boolean addEntry(final Map> entryMap, final Strin
  */
 private Map>> loadAliases(final 
ResourceResolver resolver) {
 final Map>> map = new 
ConcurrentHashMap<>();
-final String queryString = generateAliasQuery();
+final String baseQueryString = generateAliasQuery();
 
-log.debug("start alias query: {}", queryString);
-long queryStart = System.nanoTime();
-final Iterator i = resolver.findResources(queryString, 
"JCR-SQL2");
-long queryElapsed = System.nanoTime() - queryStart;
-log.debug("end alias query; elapsed {}ms", 
TimeUnit.NANOSECONDS.toMillis(queryElapsed));
+Iterator it;
+try {
+final String queryStringWithSort = baseQueryString + " AND 
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+it = new PagedQueryIterator("alias", "sling:alias", resolver, 
queryStringWithSort,
+Integer.getInteger("sling.alias.pageSize", 2000));

Review Comment:
   @reschke IIUC this is an implementation detail, which is not impacting the 
functionality in any way. It could influence performance under some 
circumstances though.
   
   I don't think that it makes sense to expose this setting via OSGI 
configuration, as it would indicate that there might be reasons to change it. 
Not sure if even the chance to override it via system properties is necessary.
   
   Can we postpone this decision until we have gathered some experience with 
it? Until then we should treat the way to change the value via system 
properties as implementation detail, which can change at any time.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2024-01-05 Thread via GitHub 


reschke commented on code in PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#discussion_r1442993651


##
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##
@@ -1148,19 +1148,26 @@ private boolean addEntry(final Map> entryMap, final Strin
  */
 private Map>> loadAliases(final 
ResourceResolver resolver) {
 final Map>> map = new 
ConcurrentHashMap<>();
-final String queryString = generateAliasQuery();
+final String baseQueryString = generateAliasQuery();
 
-log.debug("start alias query: {}", queryString);
-long queryStart = System.nanoTime();
-final Iterator i = resolver.findResources(queryString, 
"JCR-SQL2");
-long queryElapsed = System.nanoTime() - queryStart;
-log.debug("end alias query; elapsed {}ms", 
TimeUnit.NANOSECONDS.toMillis(queryElapsed));
+Iterator it;
+try {
+final String queryStringWithSort = baseQueryString + " AND 
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+it = new PagedQueryIterator("alias", "sling:alias", resolver, 
queryStringWithSort,
+Integer.getInteger("sling.alias.pageSize", 2000));

Review Comment:
   @joerghoh and @cziegeler - what do you think?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2024-01-04 Thread via GitHub 


reschke commented on code in PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#discussion_r1441754044


##
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##
@@ -1148,19 +1148,26 @@ private boolean addEntry(final Map> entryMap, final Strin
  */
 private Map>> loadAliases(final 
ResourceResolver resolver) {
 final Map>> map = new 
ConcurrentHashMap<>();
-final String queryString = generateAliasQuery();
+final String baseQueryString = generateAliasQuery();
 
-log.debug("start alias query: {}", queryString);
-long queryStart = System.nanoTime();
-final Iterator i = resolver.findResources(queryString, 
"JCR-SQL2");
-long queryElapsed = System.nanoTime() - queryStart;
-log.debug("end alias query; elapsed {}ms", 
TimeUnit.NANOSECONDS.toMillis(queryElapsed));
+Iterator it;
+try {
+final String queryStringWithSort = baseQueryString + " AND 
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+it = new PagedQueryIterator("alias", "sling:alias", resolver, 
queryStringWithSort,
+Integer.getInteger("sling.alias.pageSize", 2000));

Review Comment:
   Not necessarily; more debugging and performance testing may be needed.
   
   In any case: this is consistent with what we do for vanity paths.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2023-12-21 Thread via GitHub 


rombert commented on code in PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#discussion_r1434026128


##
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##
@@ -1148,19 +1148,26 @@ private boolean addEntry(final Map> entryMap, final Strin
  */
 private Map>> loadAliases(final 
ResourceResolver resolver) {
 final Map>> map = new 
ConcurrentHashMap<>();
-final String queryString = generateAliasQuery();
+final String baseQueryString = generateAliasQuery();
 
-log.debug("start alias query: {}", queryString);
-long queryStart = System.nanoTime();
-final Iterator i = resolver.findResources(queryString, 
"JCR-SQL2");
-long queryElapsed = System.nanoTime() - queryStart;
-log.debug("end alias query; elapsed {}ms", 
TimeUnit.NANOSECONDS.toMillis(queryElapsed));
+Iterator it;
+try {
+final String queryStringWithSort = baseQueryString + " AND 
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+it = new PagedQueryIterator("alias", "sling:alias", resolver, 
queryStringWithSort,
+Integer.getInteger("sling.alias.pageSize", 2000));

Review Comment:
   When we should remove this, right?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2023-12-21 Thread via GitHub 


reschke commented on code in PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#discussion_r1434015830


##
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##
@@ -1148,19 +1148,26 @@ private boolean addEntry(final Map> entryMap, final Strin
  */
 private Map>> loadAliases(final 
ResourceResolver resolver) {
 final Map>> map = new 
ConcurrentHashMap<>();
-final String queryString = generateAliasQuery();
+final String baseQueryString = generateAliasQuery();
 
-log.debug("start alias query: {}", queryString);
-long queryStart = System.nanoTime();
-final Iterator i = resolver.findResources(queryString, 
"JCR-SQL2");
-long queryElapsed = System.nanoTime() - queryStart;
-log.debug("end alias query; elapsed {}ms", 
TimeUnit.NANOSECONDS.toMillis(queryElapsed));
+Iterator it;
+try {
+final String queryStringWithSort = baseQueryString + " AND 
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+it = new PagedQueryIterator("alias", "sling:alias", resolver, 
queryStringWithSort,
+Integer.getInteger("sling.alias.pageSize", 2000));

Review Comment:
   The system property was added to ease debugging and performance testing, not 
to be set in practice.
   
   I seriously doubt that adding more OSGi toggles would be helpful, but if we 
did this, we should do it consistently (and maybe dissuss whether alias and 
vanity path need different ones).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2023-12-21 Thread via GitHub 


rombert commented on code in PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#discussion_r1434012781


##
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java:
##
@@ -1148,19 +1148,26 @@ private boolean addEntry(final Map> entryMap, final Strin
  */
 private Map>> loadAliases(final 
ResourceResolver resolver) {
 final Map>> map = new 
ConcurrentHashMap<>();
-final String queryString = generateAliasQuery();
+final String baseQueryString = generateAliasQuery();
 
-log.debug("start alias query: {}", queryString);
-long queryStart = System.nanoTime();
-final Iterator i = resolver.findResources(queryString, 
"JCR-SQL2");
-long queryElapsed = System.nanoTime() - queryStart;
-log.debug("end alias query; elapsed {}ms", 
TimeUnit.NANOSECONDS.toMillis(queryElapsed));
+Iterator it;
+try {
+final String queryStringWithSort = baseQueryString + " AND 
FIRST([sling:alias]) > '%s' ORDER BY FIRST([sling:alias])";
+it = new PagedQueryIterator("alias", "sling:alias", resolver, 
queryStringWithSort,
+Integer.getInteger("sling.alias.pageSize", 2000));

Review Comment:
   This should come as an OSGi config, not as a system property. OSGi 
configuration properties can then be picked up from environment variables if 
desired, see 
https://github.com/apache/felix-dev/blob/master/configadmin-plugins/interpolation/README.md#interpolating-environment-variables
 .
   
   The OSGi config should have that default of 2000 set.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2023-12-21 Thread via GitHub 


sonarcloud[bot] commented on PR #111:
URL: 
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/111#issuecomment-1866158129

   ## [![Quality Gate 
Failed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-failed-20px.png
 'Quality Gate 
Failed')](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-resourceresolver=111)
 **Quality Gate failed**  
   Failed conditions
   
   [78.9% Coverage on New 
Code](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-resourceresolver=111=new_coverage=list)
 (required ≥ 80%)  
 
   [See analysis details on 
SonarCloud](https://sonarcloud.io/dashboard?id=apache_sling-org-apache-sling-resourceresolver=111)
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] SLING-11799: ResourceResolver - use keyset pagination for sling:alias query [sling-org-apache-sling-resourceresolver]

2023-12-21 Thread via GitHub 


reschke closed pull request #110: SLING-11799: ResourceResolver - use keyset 
pagination for sling:alias query
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/110


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org