Clean up and fix task executor type hierarchy #4760

trustin · 2023-03-17T08:03:22Z

Motivation:

There are a few issues in our task executor type hierarchy.

We don't provide the context-aware version of the following task executor types:
- Executor -> ContextAwareExecutor
- ExecutorService -> ContextAwareExecutorService
- BlockingTaskExecutor -> ContextAwareBlockingTaskExecutor
There are no RequestContext.makeContextAware() and makeContextPropagating()
that accept the aforementioned executor types.
blockingTaskExecutor properties often use ScheduledExecutorService
rather than BlockingTaskExecutor.

Modifications:

Added ContextAwareExecutor.
Made ContextAwareExecutorService public.
Added ContxtAwareBlockingTaskExecutor.
Added the implementation of the aforementioned types.
Added the context-propagating version of the aforementioned types.
Merged common.RequestContextUtil to internal.common.RequestContextUtil
for less confusion.
Added BlockingTaskExecutor.of(ScheduledExecutorService).
Changed the type of blockingTaskExecutor from ScheduledExecutorService.
Added an overloaded builder method blockingTaskExecutor(BlockingTaskExecutor).
Removed the problematic wrapping of blockingTaskExecutor in
DefaultServerConfig.monitorBlockingTaskExecutor().
- The wrapped executor is only used when a user gets the executor via
  ServerConfig.blockingTaskExecutor(), which is often not the case.
  A user gets the executor via ServiceRequestContext.blockingTaskExecutor()
  and thus the wrapped executor is never used.

Result:

API completeness
- You can now create a BlockingTaskExecutor by wrapping a ScheduledExecutorService.
- You can now create context-aware or context-propagating Executor and
  BlockingTaskExecutor.
(Breaking change) The type of blockingTaskExecutor property has been
changed from ScheduledExecutorService to BlockingTaskExecutor.
- Simply recompiling your code should be enough in most cases because
  BlockingTaskExecutor is a ScheduledExecutorService.
(Breaking change) The return types of RequestContext.makeContextAware() methods
have been changed to the context-aware types, e.g.
- makeContextAware(Executor) returns ContextAwareExecutor instead of Executor.
- makeContextAware(ScheduledExecutorService) returns ContextAwareScheduledExecutorService instead of ScheduledExecutorService.

Motivation: There are a few issues in our task executor type hierarchy. - We don't provide the context-aware version of the following task executor types: - `Executor` -> `ContextAwareExecutor` - `ExecutorService` -> `ContextAwareExecutorService` - `BlockingTaskExecutor` -> `ContextAwareBlockingTaskExecutor` - There are no `RequestContext.makeContextAware()` and `makeContextPropagating()` that accept the aforementioned executor types. - `blockingTaskExecutor` properties often use `ScheduledExecutorService` rather than `BlockingTaskExecutor`. Modifications: - Added `ContextAwareExecutor`. - Made `ContextAwareExecutorService` public. - Added `ContxtAwareBlockingTaskExecutor`. - Added the implementation of the aforementioned types. - Added the context-propagating version of the aforementioned types. - Merged `common.RequestContextUtil` to `internal.common.RequestContextUtil` for less confusion. - Added `BlockingTaskExecutor.of(ScheduledExecutorService)`. - Changed the type of `blockingTaskExecutor` from `ScheduledExecutorService`. - Added an overloaded builder method `blockingTaskExecutor(BlockingTaskExecutor)`. - Removed the problematic wrapping of `blockingTaskExecutor` in `DefaultServerConfig.monitorBlockingTaskExecutor()`. - The wrapped executor is only used when a user gets the executor via `ServerConfig.blockingTaskExecutor()`, which is often not the case. A user gets the executor via `ServiceRequestContext.blockingTaskExecutor()` and thus the wrapped executor is never used. Result: - API completeness - (Breaking change) The type of `blockingTaskExecutor` property has been changed from `ScheduledExecutorService` to `BlockingTaskExecutor`. - Simply recompiling your code should be enough in most cases because `BlockingTaskExecutor` is a `ScheduledExecutorService`.

trustin · 2023-03-17T08:06:09Z

core/src/main/java/com/linecorp/armeria/common/AbstractContextAwareBlockingTaskExecutor.java

Bad rename detection

trustin · 2023-03-17T08:06:42Z

core/src/main/java/com/linecorp/armeria/common/AbstractContextAwareBlockingTaskExecutor.java

@@ -15,14 +15,12 @@
 */
 package com.linecorp.armeria.common;

-final class RequestContextUtil {


This class has been merged to com.linecorp.armeria.internal.common.RequestContextUtil to avoid confusion.

trustin · 2023-03-17T08:07:53Z

core/src/main/java/com/linecorp/armeria/common/AbstractContextAwareExecutor.java

Large portion of this class has been pulled up from AbstractContextAwareExecutorService.

minwoox

Looks good all in all! 😄

core/src/main/java/com/linecorp/armeria/common/AbstractContextAwareExecutor.java

core/src/main/java/com/linecorp/armeria/common/util/BlockingTaskExecutor.java

core/src/main/java/com/linecorp/armeria/common/RequestContext.java

core/src/main/java/com/linecorp/armeria/common/util/BlockingTaskExecutor.java

core/src/main/java/com/linecorp/armeria/common/ContextAwareExecutor.java

core/src/main/java/com/linecorp/armeria/common/RequestContext.java

ikhoon

Overall looks good. I'm happy with these changes if TimedScheduledExecutorService metrics are resolved.

ikhoon · 2023-03-28T11:28:06Z

core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.java

-        executor = new TimedScheduledExecutorService(meterRegistry, executor,
-                                                     "blockingTaskExecutor", "armeria.",
-                                                     ImmutableList.of());


Some users might be surprised that TimedScheduledExecutorService-related metrics will disappear from their monitoring dashboard. What do you think of adding TimedBlockingTaskExecutor by extending TimedScheduledExecutorService?

It's very likely that those metrics are all zero already because the executor returned by ctx.blockingTaskExecutor() is not a TimedScheduledExecutorService. 😱 How about having a separate PR for 1.24?

🙀 I didn't know that execution times are not recorded already.
If it is not a regression made by this PR, it makes sense to handle that separately.

ikhoon

Thanks for cleaning up! 🙇‍♂️ 👍

minwoox

Thanks a lot! 👍

jrhee17

Left some minor comments but looks good to me! Thanks for the cleanup @trustin ! 🙇 👍 🙇

jrhee17 · 2023-03-28T13:55:04Z

core/src/main/java/com/linecorp/armeria/common/RequestContext.java

+     * If this executor is only used from a single request then it's better to use
+     * {@link #makeContextAware(Executor)}
+     */
+    static Executor makeContextPropagating(Executor executor) {


I understood that Propagating* is different from ContextAware* in that it doesn't have store a RequestContext and thus doesn't have additional methods to expose. (i.e. ContextAwareExecutor#context). This is why we return the Executor interface as opposed to a custom interface like the ContextAware* series does.

I suppose technically Propagating* can also share a #withoutContext method, but I guess there might not be much use-cases.

I think the current approach is fine, but just wanted to point this out 👍

jrhee17 · 2023-03-28T14:17:33Z

core/src/main/java/com/linecorp/armeria/common/ContextAwareExecutor.java

+     * Returns the {@link Executor} that executes the submitted tasks without setting
+     * the {@link RequestContext}.
+     */
+    Executor withoutContext();


Not strong on this, but I'm wondering if unwrap could be better naming to match that of BlockingTaskExecutor#unwrap. (in the case of ContextAwareBlockingTaskExecutor, both unwrap and withoutContext will be available which I think may return the same executor anyways)

I'm fine with either naming though 😄

withoutContext() was already there in other public subtypes such as ContextAwareScheduledExecutorService, so I'd like to keep it consistent. I also think unwrap() doesn't convey this method's original intention - 'return the version that doesn't propagate context'.

minwoox · 2023-03-29T08:50:17Z

Thanks! 👍 👍 👍

trustin · 2023-03-29T09:02:42Z

Thanks a lot for merging this in.

trustin requested review from ikhoon, jrhee17 and minwoox as code owners March 17, 2023 08:03

trustin changed the title ~~Clean up task executor type hierarchy~~ Clean up and fix task executor type hierarchy Mar 17, 2023

trustin added new feature cleanup breaking change labels Mar 17, 2023

trustin added this to the 1.23.0 milestone Mar 17, 2023

trustin commented Mar 17, 2023

View reviewed changes

core/src/main/java/com/linecorp/armeria/common/AbstractContextAwareBlockingTaskExecutor.java

Copy link

Member Author

trustin Mar 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad rename detection

trustin commented Mar 17, 2023

View reviewed changes

Fixes

df7152c

trustin force-pushed the context_aware_blocking_task_executor branch from bfb1f04 to df7152c Compare March 17, 2023 08:19

minwoox reviewed Mar 23, 2023

View reviewed changes

Trigger build (Mon Mar 27 18:09:30 KST 2023)

aa4de4c

ikhoon reviewed Mar 27, 2023

View reviewed changes

trustin added 2 commits March 28, 2023 17:35

Merge branch 'main' into context_aware_blocking_task_executor

56ee630

Address the comments from @minwoox and @ikhoon

e6bc07e

trustin requested review from ikhoon and minwoox March 28, 2023 08:37

Fix Checkstyle violations

cb13cb6

trustin force-pushed the context_aware_blocking_task_executor branch from 46ed94b to cb13cb6 Compare March 28, 2023 08:48

ikhoon reviewed Mar 28, 2023

View reviewed changes

ikhoon approved these changes Mar 28, 2023

View reviewed changes

minwoox approved these changes Mar 28, 2023

View reviewed changes

jrhee17 approved these changes Mar 28, 2023

View reviewed changes

minwoox merged commit 787467f into line:main Mar 29, 2023

trustin deleted the context_aware_blocking_task_executor branch March 29, 2023 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up and fix task executor type hierarchy #4760

Clean up and fix task executor type hierarchy #4760

trustin commented Mar 17, 2023 •

edited

Loading

trustin Mar 17, 2023

trustin Mar 17, 2023

trustin Mar 17, 2023 •

edited

Loading

minwoox left a comment

ikhoon left a comment

ikhoon Mar 28, 2023

trustin Mar 28, 2023

ikhoon Mar 28, 2023

ikhoon left a comment

minwoox left a comment

jrhee17 left a comment

jrhee17 Mar 28, 2023

jrhee17 Mar 28, 2023

trustin Mar 29, 2023

minwoox commented Mar 29, 2023

trustin commented Mar 29, 2023

Clean up and fix task executor type hierarchy #4760

Clean up and fix task executor type hierarchy #4760

Conversation

trustin commented Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox commented Mar 29, 2023

trustin commented Mar 29, 2023

trustin commented Mar 17, 2023 •

edited

Loading

trustin Mar 17, 2023 •

edited

Loading