You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@sling.apache.org by GitBox <gi...@apache.org> on 2020/07/20 06:48:21 UTC
[GitHub] [sling-org-apache-sling-distribution-journal-messages] amit-jain opened a new pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
amit-jain opened a new pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4
- Introduced an interface BinaryStore and JcrBinaryStore as the default implementation storing the binaries in JCR with no change in the existing strategy of storing based on size.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] cschneider commented on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
cschneider commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-662106453
Yes you are rifght, I forgot about "id". It also makes sense to add it as it makes the API idempotent.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] amit-jain commented on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
amit-jain commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-662292973
@cschneider @tmaret Have updated the PR to use the simplified interface.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] amit-jain commented on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
amit-jain commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-660945195
Yes, the simple apis won't work with jcr at least looks like cleanly. To have more clean implementation for support of default JCR as well as other stores please see implementation at https://github.com/apache/sling-org-apache-sling-distribution-journal/pull/54.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] cschneider edited a comment on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
cschneider edited a comment on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-660939060
Do you think we need the binary store to be generic?
Could we just make it like this ?
public interface BinaryStore {
InputStream get(String url) throws IOException;
String store(InputStream binaryStream) throws IOException;
}
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] cschneider edited a comment on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
cschneider edited a comment on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-661918394
I created a PR to change the PackageRepo so it does not need ResourceResolver and DistributionPackage anymore. https://github.com/apache/sling-org-apache-sling-distribution-journal/pull/55
The simpler interface would be :
public interface BinaryStore {
InputStream get(String id) throws IOException;
String put(String id, InputStream is, long length) throws IOException;
}
In comparison to the interface I originally thought of, Amit added the length parameter which makes sense as Azure blob store needs it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] cschneider merged pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
cschneider merged pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] cschneider commented on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
cschneider commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-660939060
Do you think we need the binary store to be generic. Could we just make it:
public interface BinaryStore {
InputStream get(String url) throws IOException;
String store(InputStream binaryStream) throws IOException;
}
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] tmaret commented on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
tmaret commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-661910485
It makes sense to avoid ResourceResolver as it's tied to the Sling repository and we will likely store packages not managed by Sling (e.g. Azure blobs, S3, etc.).
What is the proposed alternative API exactly ?
Regarding the transition, storing the packages under a different path would indeed only affect the collection of the packages. It would not impact importing the packages because those are resolved at the JCR level with references pointing to the blob store. As pointed out by @cschneider, barely any packages are stored in JCR anyway, we added that path for compatibility reason, knowing it'd be barely used. Our 1500+ cloud environment exhibit 0 occurrence over last week for instance.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] sonarcloud[bot] commented on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-662291793
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-
distribution-journal-messages&pullRequest=4&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo.png' alt='No Coverage information' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&metric=coverage&view=list) No Coverage information
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] cschneider commented on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
cschneider commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-660951935
I know we are currently using the ResourceResolver in String store(ResourceResolver resolver, DistributionPackage disPkg). I am not sure it is necessary though.
PackageRepo already has a reference to ResourceResolverFactory. So we could create our own ResourceResolver to store the package.
I also don't think we strictly need to store the packages by package type. If we store them just in one path we do not need to get the pkg type.
So I think with a little more effort we could remove the need for ResourceResolver and for DistributionPackage.
As we introduce a possibly long lasting API with BinaryStore I think it is worth the additional effort.
One thing to consider is of course how to make the transition. If we change the way we store we might leave some old packages behind. As we use the PackageRepo only very seldomly I think we could just leave possible old packages behind and clean them up manually if needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] sonarcloud[bot] commented on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-660838920
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-
distribution-journal-messages&pullRequest=4&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo.png' alt='No Coverage information' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&metric=coverage&view=list) No Coverage information
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] sonarcloud[bot] removed a comment on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-660838920
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-
distribution-journal-messages&pullRequest=4&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=SECURITY_HOTSPOT) to review)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo.png' alt='No Coverage information' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&metric=coverage&view=list) No Coverage information
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-distribution-journal-messages&pullRequest=4&metric=new_duplicated_lines_density&view=list)
<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/message_warning.png' alt='warning' width='16' height='16' /> The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more [here](https://sonarcloud.io/documentation/upcoming/)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] cschneider commented on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
cschneider commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-661918394
I created a PR to change the PackageRepo so it does not need ResourceResolver and DistributionPackage anymore. https://github.com/apache/sling-org-apache-sling-distribution-journal/pull/55
The simpler interface would be :
public interface BinaryStore {
InputStream get(String id) throws IOException;
String put(String id, InputStream is, long length)
throws IOException;
}
In comparison to the interface I originally thought of, Amit added the length parameter which makes sense as Azure blob store needs it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-distribution-journal-messages] tmaret commented on pull request #4: SLING-9593: Abstract out binary handling to allow plugging of different stores
Posted by GitBox <gi...@apache.org>.
tmaret commented on pull request #4:
URL: https://github.com/apache/sling-org-apache-sling-distribution-journal-messages/pull/4#issuecomment-662038636
Content-Length header is indeed useful to upload a size delimited payloads efficiently to an HTTP API. It is true for S3 as well. It makes sense to be in the API.
With the addition of the `length` and the `identifier` parameters, the API looks good to me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org