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 2021/06/04 09:26:58 UTC
[GitHub] [sling-org-apache-sling-feature] kwin opened a new pull request #25: SLING-10457 make ArtifactProvider implement Closeable
kwin opened a new pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25
pass exceptions for errors during calling close()
--
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-feature] kwin commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646117326
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
New implementation should only implement close(). Therefore the default impl of the deprecated method.
--
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-feature] kwin commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646146539
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
The idea is that provider implementations rather implement `close()` instead of `shutdown()` to be able to propagate IOExceptions. Having two different methods for shutting down and only the suboptimal signature (`shutdown()`) as mandatory implementation feels wrong. With regards to no shutdown/close method being mandatory I think this totally fine, as not every artifact provider necessarily needs to do clean up.
--
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-feature] kwin commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646435669
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
All you said is true, but my argument was "To log all those places centrally, the exceptions should hit the manager.". Do you disagree with that?
Just to clarify: I want to get rid of https://github.com/apache/sling-org-apache-sling-feature/blob/3d6749494d3b5cc180aed627e207509061ad0be0/src/main/java/org/apache/sling/feature/io/artifacts/ArtifactManager.java#L371 and let the exception bubble up to the caller. In the case of a Maven plugin you should probably log with something or even fail the build in case the clean fails.
--
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-feature] kwin commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646435669
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
All you said is true, but my argument was "To log all those places centrally, the exceptions should hit the manager.". Do you disagree with that?
--
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-feature] cziegeler commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646426608
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
No, AM implements close without throwing an exception, see https://github.com/apache/sling-org-apache-sling-feature/blob/master/src/main/java/org/apache/sling/feature/io/artifacts/ArtifactManager.java#L126
AM also implements AutoCloseable - so AM can already be used in a try-with-resources block. Nothing needs to change there, and we can apply the same pattern to the ArtifactProvider - let it implement AutoCloseable with a default method close() not throwing an exception and calling shutdown.
And as mentioned I don't think that the exceptions should hit the manager - why, what should the manager do about them?
--
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-feature] cziegeler commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646272303
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
I'm not sure - shutdown not throwing a checked exception is intentional - what should a client do if shutdown is failing? I think shutdown is different from closing an output stream where close is required to fully work to not loose data. If shutdown of the provider fails - what damage does it do?
--
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-feature] cziegeler commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646438279
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
Yes I disagree :) using exceptions just to ensure central logging sounds wrong 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
[GitHub] [sling-org-apache-sling-feature] kwin commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646407428
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
> With introducing close() to both, the provider and the manager, you create this new situation where the provider now can throw on close - which then needs to be handled by the manager.
AM had a close() method before this PR. So it may have thrown IOExceptions already there. The only change right now is that providers (instead of just logging with their own logging framework) propagate their exception to the manager. Although this doesn't matter that much if you only think about the Default provider, it may make a difference for 3rd party providers. To log all those places centrally, the exceptions should hit the manager.
--
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-feature] kwin commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646435669
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
All you said is true, but my argument was "To log all those places centrally, the exceptions should hit the manager.". Do you disagree with that?
Just to clarify: I want to get rid of https://github.com/apache/sling-org-apache-sling-feature/blob/3d6749494d3b5cc180aed627e207509061ad0be0/src/main/java/org/apache/sling/feature/io/artifacts/ArtifactManager.java#L371 and let the exception bubble up to the caller. In the case of a Maven plugin you should probably log with something or even fail the build in case the close/shutdown fails.
--
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-feature] cziegeler commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646383549
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
But shouldn't such a retry logic be part of shutdown? It seems to be a high burden to clients to figure out whether the IOException is transient or persistent.
Right now, both provider and manager have a shutdown method not throwing a checked exception. It is expected that the implementation does whatever is required to shutdown. With introducing close() to both, the provider and the manager, you create this new situation where the provider now can throw on close - which then needs to be handled by the manager.
Therefore my expectation is that shutdown never throws (a checked exception), we can implement Closeable as a convenience but close() will never throw an 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-feature] kwin commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646435669
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
All you said is true, but my argument was "To log all those places centrally, the exceptions should hit the manager.". Do you disagree with that?
Just to clarify: I want to get rid of https://github.com/apache/sling-org-apache-sling-feature/blob/3d6749494d3b5cc180aed627e207509061ad0be0/src/main/java/org/apache/sling/feature/io/artifacts/ArtifactManager.java#L371 and let the exception bubble up to the caller. In the case of a Maven plugin you should probably log with the Mojo logger or even fail the build in case the close/shutdown fails.
--
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-feature] kwin commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646289119
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
The ArtifactManager is treating IOExceptions differently than other unchecked exceptions (https://github.com/apache/sling-org-apache-sling-feature/pull/25/files#diff-36fc363deea378f4f1f538a997cebb80f6aca64974b2aef836f91371c44e06ecR114 and https://github.com/apache/sling-org-apache-sling-feature/pull/25/files#diff-36fc363deea378f4f1f538a997cebb80f6aca64974b2aef836f91371c44e06ecR143) to not hide the original exception/still close the other providers. Also since all File based operations might throw IOE in Java otherwise the provider would need to wrap it in a RuntimeException (if implementing shutdown).
In certain cases it might make sense to retry calling close() as some IOExceptions are transient....
--
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-feature] sonarcloud[bot] commented on pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#issuecomment-855210621
SonarCloud Quality Gate failed.
[<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-feature&pullRequest=25&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-feature&pullRequest=25&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature&pullRequest=25&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-feature&pullRequest=25&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-feature&pullRequest=25&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature&pullRequest=25&resolved=false&types=VULNERABILITY)
[<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/security_hotspots?id=apache_sling-org-apache-sling-feature&pullRequest=25&resolved=false&types=SECURITY_HOTSPOT) [<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/security_hotspots?id=apache_sling-org-apache-sling-feature&pullRequest=25&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-feature&pullRequest=25&resolved=false&types=SECURITY_HOTSPOT)
[<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-feature&pullRequest=25&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-feature&pullRequest=25&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-feature&pullRequest=25&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature&pullRequest=25&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature&pullRequest=25&metric=new_coverage&view=list)
[<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-feature&pullRequest=25&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-feature&pullRequest=25&metric=new_duplicated_lines_density&view=list)
--
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-feature] cziegeler commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646114728
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
Why is this switching to a default implementation?
--
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-feature] cziegeler commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646119553
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
But as close is also a default method (which makes sense for compatibility) this is kind of a strange situation and can easily lead to that none of them is implemented.
I have the feeling we should not deprecate shutdown but just add close calling shutdown to suppot Closeable
--
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-feature] cziegeler commented on a change in pull request #25: SLING-10457 make ArtifactProvider implement Closeable
Posted by GitBox <gi...@apache.org>.
cziegeler commented on a change in pull request #25:
URL: https://github.com/apache/sling-org-apache-sling-feature/pull/25#discussion_r646426608
##########
File path: src/main/java/org/apache/sling/feature/io/artifacts/spi/ArtifactProvider.java
##########
@@ -43,8 +44,20 @@
/**
* Shutdown the provider.
+ * @deprecated Use {@link #close()} instead.
*/
- void shutdown();
+ @Deprecated
+ default void shutdown() {
+
Review comment:
No, AM implements close without throwing an exception, see https://github.com/apache/sling-org-apache-sling-feature/blob/master/src/main/java/org/apache/sling/feature/io/artifacts/ArtifactManager.java#L126
AM also implements AutoCloseable - so AM can already be used in a try-with-resources block. Nothing needs to change there, and we can apply the same pattern to the ArtifactProvider - let it implement AutoCloseable with a default method close() not throwing an exception and calling shutdown
--
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