You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2022/03/09 15:29:34 UTC

[GitHub] [sling-org-apache-sling-starter] jsedding opened a new pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

jsedding opened a new pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59


   - set minimum cardinality for contentloader readers to 4


-- 
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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] kwin commented on a change in pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#discussion_r822777481



##########
File path: src/main/features/base.json
##########
@@ -358,6 +358,10 @@
             "user.mapping":[
                 "org.apache.sling.jcr.contentloader=[sling-jcr-content-loader]"
             ]
+        },
+        "org.apache.sling.jcr.contentloader.internal.ContentReaderWhiteboard": {
+            // SLING-11168 - ensure all four ootb readers are started
+            "contentReader.cardinality.minimum": 4

Review comment:
       I am in favour of a fix which doesn't require additional OSGi configuration (like https://github.com/apache/sling-org-apache-sling-jcr-contentloader/pull/12) in order for other distributions to also benefit from it automatically (without having to configure anything explicitly).




-- 
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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] stefanseifert commented on a change in pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
stefanseifert commented on a change in pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#discussion_r822798054



##########
File path: src/main/features/base.json
##########
@@ -358,6 +358,10 @@
             "user.mapping":[
                 "org.apache.sling.jcr.contentloader=[sling-jcr-content-loader]"
             ]
+        },
+        "org.apache.sling.jcr.contentloader.internal.ContentReaderWhiteboard": {
+            // SLING-11168 - ensure all four ootb readers are started
+            "contentReader.cardinality.minimum": 4

Review comment:
       i do not like the approach with "minimum=4" (wherever it is defined) - this is too subtle and will likely be forgotten if someone adds a 5th content reader implementation in the future - and then it may produce unreliable results again when it may happen that one random of the 5 readers is not ready. it may also be unreliable when there are custom content readers in the system from other bundles.




-- 
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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] stefanseifert commented on a change in pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
stefanseifert commented on a change in pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#discussion_r823695462



##########
File path: src/main/features/base.json
##########
@@ -358,6 +358,10 @@
             "user.mapping":[
                 "org.apache.sling.jcr.contentloader=[sling-jcr-content-loader]"
             ]
+        },
+        "org.apache.sling.jcr.contentloader.internal.ContentReaderWhiteboard": {
+            // SLING-11168 - ensure all four ootb readers are started
+            "contentReader.cardinality.minimum": 4

Review comment:
       that would be possible, but is it worth taking those extra-roundtrip? i think we are very close to the final solution, we have even two PRs in place which actually fix the problem. once we have the final solution ready we could release a new contentloader bundle version together with the final sling 12 starter?




-- 
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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] kwin commented on a change in pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#discussion_r822784062



##########
File path: src/main/features/base.json
##########
@@ -358,6 +358,10 @@
             "user.mapping":[
                 "org.apache.sling.jcr.contentloader=[sling-jcr-content-loader]"
             ]
+        },
+        "org.apache.sling.jcr.contentloader.internal.ContentReaderWhiteboard": {
+            // SLING-11168 - ensure all four ootb readers are started
+            "contentReader.cardinality.minimum": 4

Review comment:
       Can't we make the cardinality minimum the default in the ContentReaderWhiteboard?




-- 
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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] rombert commented on a change in pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#discussion_r823680920



##########
File path: src/main/features/base.json
##########
@@ -358,6 +358,10 @@
             "user.mapping":[
                 "org.apache.sling.jcr.contentloader=[sling-jcr-content-loader]"
             ]
+        },
+        "org.apache.sling.jcr.contentloader.internal.ContentReaderWhiteboard": {
+            // SLING-11168 - ensure all four ootb readers are started
+            "contentReader.cardinality.minimum": 4

Review comment:
       My understanding is that the contentloader fix is still under review, with an alternate proposal at https://github.com/apache/sling-org-apache-sling-jcr-contentloader/pull/13 . 
   
   I would suggest that we merge this fix for Starter 12, revert it after the release, and then update to the latest JCR ContentLoader release once we have a fix.
   
   How does that sound?




-- 
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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] jsedding commented on a change in pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
jsedding commented on a change in pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#discussion_r823709936



##########
File path: src/main/features/base.json
##########
@@ -358,6 +358,10 @@
             "user.mapping":[
                 "org.apache.sling.jcr.contentloader=[sling-jcr-content-loader]"
             ]
+        },
+        "org.apache.sling.jcr.contentloader.internal.ContentReaderWhiteboard": {
+            // SLING-11168 - ensure all four ootb readers are started
+            "contentReader.cardinality.minimum": 4

Review comment:
       I agree this should not be a permanent fix. It may or may not be a good solution to unblock the Sling 12 release.




-- 
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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] rombert commented on a change in pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
rombert commented on a change in pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#discussion_r823697047



##########
File path: src/main/features/base.json
##########
@@ -358,6 +358,10 @@
             "user.mapping":[
                 "org.apache.sling.jcr.contentloader=[sling-jcr-content-loader]"
             ]
+        },
+        "org.apache.sling.jcr.contentloader.internal.ContentReaderWhiteboard": {
+            // SLING-11168 - ensure all four ootb readers are started
+            "contentReader.cardinality.minimum": 4

Review comment:
       I haven't followed the conversations closely. If we think are close to a final solution, then we can wait for a couple of days more, sure.




-- 
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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] kwin commented on a change in pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#discussion_r822784062



##########
File path: src/main/features/base.json
##########
@@ -358,6 +358,10 @@
             "user.mapping":[
                 "org.apache.sling.jcr.contentloader=[sling-jcr-content-loader]"
             ]
+        },
+        "org.apache.sling.jcr.contentloader.internal.ContentReaderWhiteboard": {
+            // SLING-11168 - ensure all four ootb readers are started
+            "contentReader.cardinality.minimum": 4

Review comment:
       Can't we make the cardinality minimum the default in the ContentReaderWhiteboard? This should work via http://docs.osgi.org/specification/osgi.cmpn/7.0.0/service.component.html#org.osgi.service.component.annotations.Component.property-- set in https://github.com/apache/sling-org-apache-sling-jcr-contentloader/blob/7b42e0e0f390e8cd29a9ba235e4e19808f011e7e/src/main/java/org/apache/sling/jcr/contentloader/internal/ContentReaderWhiteboard.java#L29




-- 
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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] kwin commented on a change in pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#discussion_r822796241



##########
File path: src/main/features/base.json
##########
@@ -358,6 +358,10 @@
             "user.mapping":[
                 "org.apache.sling.jcr.contentloader=[sling-jcr-content-loader]"
             ]
+        },
+        "org.apache.sling.jcr.contentloader.internal.ContentReaderWhiteboard": {
+            // SLING-11168 - ensure all four ootb readers are started
+            "contentReader.cardinality.minimum": 4

Review comment:
       I still prefer the fix from https://github.com/apache/sling-org-apache-sling-jcr-contentloader/pull/12/files as it requires the 4 relevant content readers (not only 4 arbitrary 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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] sonarcloud[bot] commented on pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#issuecomment-1063051381


   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; ![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-starter&pullRequest=59&resolved=false&types=CODE_SMELL)
   
   [![No Coverage information](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo-16px.png 'No Coverage information')](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-starter&pullRequest=59) No Coverage information  
   [![No Duplication information](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/NoDuplicationInfo-16px.png 'No Duplication information')](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-starter&pullRequest=59&metric=duplicated_lines_density&view=list) No Duplication information
   
   


-- 
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-unsubscribe@sling.apache.org

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

[GitHub] [sling-org-apache-sling-starter] kwin commented on a change in pull request #59: SLING-11168 - Sling Starter 12: Unable to launch Composum

Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #59:
URL: https://github.com/apache/sling-org-apache-sling-starter/pull/59#discussion_r822777481



##########
File path: src/main/features/base.json
##########
@@ -358,6 +358,10 @@
             "user.mapping":[
                 "org.apache.sling.jcr.contentloader=[sling-jcr-content-loader]"
             ]
+        },
+        "org.apache.sling.jcr.contentloader.internal.ContentReaderWhiteboard": {
+            // SLING-11168 - ensure all four ootb readers are started
+            "contentReader.cardinality.minimum": 4

Review comment:
       I am in favour of a fix which doesn't require additional OSGi configuration (like https://github.com/apache/sling-org-apache-sling-jcr-contentloader/pull/12) in order for other distributions also benefit from it automatically (without having to configure anything explicitly).




-- 
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-unsubscribe@sling.apache.org

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