You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2022/10/29 18:28:10 UTC
[GitHub] [bookkeeper] merlimat opened a new pull request, #3593: Removed the necessity of SafeRunnable
merlimat opened a new pull request, #3593:
URL: https://github.com/apache/bookkeeper/pull/3593
### Motivation
`OrderedExecutor` is taking `SafeRunnable` tasks instead of the standard `Runnable`.
The reason was the Java `ThreadPoolExecutor` are failing when an exception is thrown by one of the tasks and there is no log printed of the exception.
Right now, this is not needed anymore, because the `SingleThreadExecutor` is already doing try/catch/log: https://github.com/apache/bookkeeper/blob/8ac28db16118e5415c041f8da8cca73777971ce6/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/SingleThreadExecutor.java#L135
For OrderedScheduler, we can instead wrap the tasks when they are submitted, instead of relying on caller to wrap them.
There are a few reasons not to use `SafeRunnable`:
1. It requires 1 extra allocation for each task
2. It makes the API non-standard, pushing the need to always manually wrap the regular `Runnable`
3. Even when using it in lambda-form, it still requires extra levels of indentation that make the code more difficult to read.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] hangc0276 commented on pull request #3593: Removed the necessity of SafeRunnable
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #3593:
URL: https://github.com/apache/bookkeeper/pull/3593#issuecomment-1297912364
@eolivelli Would you please take a look? thanks.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on a diff in pull request #3593: Removed the necessity of SafeRunnable
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #3593:
URL: https://github.com/apache/bookkeeper/pull/3593#discussion_r1008782207
##########
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java:
##########
@@ -526,7 +520,7 @@ public boolean preserveMdc() {
* @param orderingKey
* @param r
*/
- public void executeOrdered(Object orderingKey, SafeRunnable r) {
+ public void executeOrdered(Object orderingKey, Runnable r) {
Review Comment:
`SafeRunnable` interface is not being removed and changing these methods to require `Runnable` should be ok since it's the "parent" 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.
To unsubscribe, e-mail: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat commented on a diff in pull request #3593: Removed the necessity of SafeRunnable
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #3593:
URL: https://github.com/apache/bookkeeper/pull/3593#discussion_r1008782255
##########
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java:
##########
@@ -526,7 +520,7 @@ public boolean preserveMdc() {
* @param orderingKey
* @param r
*/
- public void executeOrdered(Object orderingKey, SafeRunnable r) {
+ public void executeOrdered(Object orderingKey, Runnable r) {
Review Comment:
eg: passing a `SafeRunnable` instance is still valid, because it implements `Runnable`.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] merlimat merged pull request #3593: Removed the necessity of SafeRunnable
Posted by GitBox <gi...@apache.org>.
merlimat merged PR #3593:
URL: https://github.com/apache/bookkeeper/pull/3593
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #3593: Removed the necessity of SafeRunnable
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #3593:
URL: https://github.com/apache/bookkeeper/pull/3593#discussion_r1008750248
##########
bookkeeper-common/src/main/java/org/apache/bookkeeper/common/util/OrderedExecutor.java:
##########
@@ -526,7 +520,7 @@ public boolean preserveMdc() {
* @param orderingKey
* @param r
*/
- public void executeOrdered(Object orderingKey, SafeRunnable r) {
+ public void executeOrdered(Object orderingKey, Runnable r) {
Review Comment:
I am pretty sure that this API is uses by other projects as utility.
Could we still keep the old methods in order to keep compatibility?
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org