You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by "andrewmkhoury (via GitHub)" <gi...@apache.org> on 2023/04/21 06:15:10 UTC
[GitHub] [sling-org-apache-sling-event] andrewmkhoury opened a new pull request, #26: SLING-11831 - Allow setting job properties for custom job state
andrewmkhoury opened a new pull request, #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26
Add setProperty to the JobExecutionContext for adding custom properties.
--
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-event] andrewmkhoury commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1181150864
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
Review Comment:
Ok, done:
https://github.com/apache/sling-org-apache-sling-event/pull/26/files#diff-6821a1c0362dd00f0e0c3dd812e85e5fc3424c1b63971fe46712d547a3eafe75R89
https://github.com/apache/sling-org-apache-sling-event/pull/26/files#diff-9c5fb3d1b7e3b0f54bc5c4182965c4fe1f9023d449017cece3005d3f90e8e4d8R122-R126
--
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-event] andrewmkhoury commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1173913659
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
Review Comment:
Since it is a user facing API, I implemented it opting for IllegalArgumentException as the exception type. Do you agree or should I change it and allow it to be an NPE?
--
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-event] klcodanr commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "klcodanr (via GitHub)" <gi...@apache.org>.
klcodanr commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1173932076
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
Review Comment:
I'm okay with keeping the current code to have a more descriptive error, but I agree with @joerghoh regarding the annotations.
--
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-event] joerghoh commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1173517103
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
+ }
+ if ( value == null ) {
+ throw new IllegalArgumentException("Value must not be null");
+ }
+ if ( name.startsWith("slingevent:") || name.startsWith(":slingevent:")) {
+ throw new IllegalArgumentException("Property name must not start with slingevent: " + name);
Review Comment:
We should make the exception message more precise and also mention ":slingegent:" as not allowed.
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
+ }
+ if ( value == null ) {
+ throw new IllegalArgumentException("Value must not be null");
+ }
+ if ( name.startsWith("slingevent:") || name.startsWith(":slingevent:")) {
+ throw new IllegalArgumentException("Property name must not start with slingevent: " + name);
Review Comment:
We should make the exception message more precise and also mention ":slingevent:" as not allowed.
--
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-event] andrewmkhoury commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1181003628
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
Review Comment:
I had thought of using `@NonNull` too but it meant I need to introduce a new dependency to this project and org.apache.sling.event.api project. WDYT?
```
<dependency>
<groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId>
<version>3.0.2</version>
</dependency>
```
Also, note that @Nonnull isn't perfect at blocking null parameters from getting passed, so it still make sense to keep the null checks. See here for details:
https://stackoverflow.com/questions/13484202/how-to-use-nullable-and-nonnull-annotations-more-effectively/46290602#46290602
--
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-event] andrewmkhoury commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1181003628
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
Review Comment:
I had thought of using `@NonNull` too, but it meant I need to introduce a new dependency to this project and org.apache.sling.event.api project. WDYT?
```
<dependency>
<groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId>
<version>3.0.2</version>
</dependency>
```
Also, note that `@Nonnull` isn't perfect at blocking null parameters from getting passed, so it still makes sense to keep the null checks. See here for details:
https://stackoverflow.com/questions/13484202/how-to-use-nullable-and-nonnull-annotations-more-effectively/46290602#46290602
--
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-event] kwin commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "kwin (via GitHub)" <gi...@apache.org>.
kwin commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1181019895
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
Review Comment:
we use other null annotations in Sling: https://sling.apache.org/documentation/development/null-analysis.html
--
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-event] andrewmkhoury commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1173911475
##########
src/main/java/org/apache/sling/event/impl/jobs/tasks/CleanUpTask.java:
##########
@@ -181,6 +181,11 @@ public void log(String message, Object... args) {
}
+ @Override
+ public void setProperty(String name, Object value) {
+
Review Comment:
All the methods of that JobExecutionContext had no comments. So I addedd a comment at the top of the class explaining why.
--
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-event] joerghoh commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1173518197
##########
src/main/java/org/apache/sling/event/impl/jobs/tasks/CleanUpTask.java:
##########
@@ -181,6 +181,11 @@ public void log(String message, Object... args) {
}
+ @Override
+ public void setProperty(String name, Object value) {
+
Review Comment:
please at least a comment here, not just a empty method body.
--
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-event] andrewmkhoury commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1173911475
##########
src/main/java/org/apache/sling/event/impl/jobs/tasks/CleanUpTask.java:
##########
@@ -181,6 +181,11 @@ public void log(String message, Object... args) {
}
+ @Override
+ public void setProperty(String name, Object value) {
+
Review Comment:
All the methods of that JobExecutionContext were empty with no comments. So I addedd a comment at the top of the class explaining why.
--
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-event] andrewmkhoury commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1181148682
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
Review Comment:
Thanks @kwin, I will use those.
--
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-event] klcodanr merged pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "klcodanr (via GitHub)" <gi...@apache.org>.
klcodanr merged PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26
--
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-event] joerghoh commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "joerghoh (via GitHub)" <gi...@apache.org>.
joerghoh commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1173922933
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
Review Comment:
We could also add a @NotNull annotation to the API to make it more visible.
--
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-event] klcodanr commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "klcodanr (via GitHub)" <gi...@apache.org>.
klcodanr commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1173908716
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
Review Comment:
Optional, this could be simplfied with:
Objects.requireNonNull(name, "Name must not be null")
But this would throw a NPE (with the supplied message) rather that an IllegalArgumentMessage.
https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#requireNonNull-T-java.lang.String-
--
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-event] andrewmkhoury commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1173911475
##########
src/main/java/org/apache/sling/event/impl/jobs/tasks/CleanUpTask.java:
##########
@@ -181,6 +181,11 @@ public void log(String message, Object... args) {
}
+ @Override
+ public void setProperty(String name, Object value) {
+
Review Comment:
All the methods of that JobExecutionContext were empty with no comments. So I addedd a comment at the top of the class explaining why.
https://github.com/apache/sling-org-apache-sling-event/pull/26/files#diff-096807b05ae963f5cc873b3e79730bac1dba15209f08e710cd5415d87e6b7e87R153-R156
--
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-event] andrewmkhoury commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1173912187
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
+ }
+ if ( value == null ) {
+ throw new IllegalArgumentException("Value must not be null");
+ }
+ if ( name.startsWith("slingevent:") || name.startsWith(":slingevent:")) {
+ throw new IllegalArgumentException("Property name must not start with slingevent: " + name);
Review Comment:
Ok, done.
--
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-event] andrewmkhoury commented on a diff in pull request #26: SLING-11831 - Allow setting job properties for custom job state
Posted by "andrewmkhoury (via GitHub)" <gi...@apache.org>.
andrewmkhoury commented on code in PR #26:
URL: https://github.com/apache/sling-org-apache-sling-event/pull/26#discussion_r1181003628
##########
src/main/java/org/apache/sling/event/impl/jobs/queues/JobExecutionContextImpl.java:
##########
@@ -84,6 +84,22 @@ public void updateProgress(final long eta) {
}
}
+ @Override
+ public void setProperty(final String name, final Object value) {
+ if ( name == null ) {
+ throw new IllegalArgumentException("Name must not be null");
Review Comment:
I had thought of using `@NonNull` too, but it meant I need to introduce a new dependency to this project and org.apache.sling.event.api project. WDYT?
```
<dependency>
<groupId>com.google.code.findbugs</groupId>
<artifactId>jsr305</artifactId>
<version>3.0.2</version>
</dependency>
```
Also, note that @Nonnull isn't perfect at blocking null parameters from getting passed, so it still make sense to keep the null checks. See here for details:
https://stackoverflow.com/questions/13484202/how-to-use-nullable-and-nonnull-annotations-more-effectively/46290602#46290602
--
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