joerghoh commented on code in PR #39:
URL:
https://github.com/apache/sling-org-apache-sling-event/pull/39#discussion_r2160845806
##########
src/main/java/org/apache/sling/event/impl/jobs/config/JobManagerConfiguration.java:
##########
@@ -308,21 +306,21 @@ public boolean isActive() {
* This ResourceResolver provides read and write access to all resources
relevant for the event
* and job handling.
*
- * @return A resource resolver or {@code null} if the component is already
deactivated.
+ * @return A resource resolver
* @throws RuntimeException if the resolver can't be created.
*/
- public ResourceResolver createResourceResolver() {
- ResourceResolver resolver = null;
+ public @NotNull ResourceResolver createResourceResolver() {
Review Comment:
These null-checks seem to be added rather by accident than really planned.
Otherwise all places where no null-checks exist would need to be augmented with
these checks as well, and that was not a problem for a very long time.
Silently swallowing these exceptions (and therefor not executing the
expected action) is also not a good pattern. On the other hand side the
LoginException happens mostly due to configuration issues (also rare, because
``createResourceResolver`` code is not exported, and thus will only be used by
the Sling event code), and therefor they should be easily fixable. So in the
end the actual type of issue does not seem to be a problem, which is likely to
happen frequently.
And for that reason I think it's possible to adjust the way errors are
handled, as this is contained in this code. We might want to add a
``Thread.UncaughtExceptionHandler`` to the threads of the job threadpool(s).
--
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
