Provide a way to invoke Thrift service impls from `BlockingTaskExecutor` #5619

ChangguHan · 2024-04-18T05:50:45Z

Related issue: #4917

Motivation:

There's currently no way to make Thrift services run from
the BlockingTaskExecutor.

Modifications:

Added useBlockingTaskExecutor property
Added ThriftCallServiceBuilder to build ThriftCallService fluently
ThriftCallService now calls the service implementation from
the BlockingTaskExecutor if configured so.

Result:

Closes Allow useBlockingTaskExecutor in Thrift services #4917
User can now specify whether to invoke the service implementation
from the BlockingTaskExecutor for Thrift services.

CLAassistant · 2024-04-18T05:50:51Z

All committers have signed the CLA.

github-actions · 2024-04-18T06:05:56Z

🔍 Build Scan® (commit: `7876ae5`)

Job name	Status	Build Scan®
build-windows-latest-jdk-21	✅	https://ge.armeria.dev/s/zyv5ychb74cyi
build-self-hosted-unsafe-jdk-8	✅	https://ge.armeria.dev/s/5gbtn7jfo3hbk
build-self-hosted-unsafe-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/mkiwhxqqwo4bc
build-self-hosted-unsafe-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/axbcmxzdpuqvy
build-self-hosted-unsafe-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/yjdw3q4vjeymi
build-self-hosted-unsafe-jdk-17-leak	❌ (failure)	https://ge.armeria.dev/s/yq5kpsjeqryju
build-self-hosted-unsafe-jdk-11	✅	https://ge.armeria.dev/s/owj3lbzt64qjw
build-macos-12-jdk-21	✅	https://ge.armeria.dev/s/pjwk7fj7hxgpq

injae-kim · 2024-04-19T01:20:35Z

thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/THttpService.java

+        if (useBlockingTaskExecutor) {
+            ctx.blockingTaskExecutor().execute(
+                    () -> invoke(ctx, serializationFormat, seqId, f, decodedReq, httpRes)
+            );


we think invoke() is only thing that we should wrap with ctx.blockingTaskExecutor().

If not, please share to us~! 🙇

thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/THttpService.java

ikhoon · 2024-04-22T11:10:47Z

...ft/thrift0.13/src/test/java/com/linecorp/armeria/server/thrift/THttpServiceBlockingTest.java

+        @Override
+        public void get(TestServiceRequest request,
+                        AsyncMethodCallback resultHandler) throws TException {
+            resultHandler.onComplete(new TestServiceResponse(request.getMessage()));


Could we check whether the current thread is the blocking task executor?

Should we also check TestService.Iface?

Could we check whether the current thread is the blocking task executor?

I checked it here

armeria/thrift/thrift0.13/src/test/java/com/linecorp/armeria/server/thrift/THttpServiceBlockingTest.java

Line 81 in 6649eee

assertThat(blocking).isFalse();

Should we also check TestService.Iface?

Fixed it. Because of Iface, only verified the current thread is blocking executor without adding useBlockingTaskExecutor

thrift/thrift0.13/src/test/thrift/Test.thrift

minwoox

Looks great to me. 👍
Left minor suggestions. 😉

...ft/thrift0.13/src/test/java/com/linecorp/armeria/server/thrift/THttpServiceBlockingTest.java

thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/THttpService.java

minwoox

Looks great to me, @ChangguHan 👍 👍 👍
Thanks!

minwoox · 2024-04-23T11:55:48Z

Oops, I realized that we also need to fix ThriftCallService not to use blocking task executor twice:

armeria/thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallService.java

Line 186 in 10424c7

ctx.blockingTaskExecutor().execute(() -> {

private static void invoke(
        ServiceRequestContext ctx,
        Object impl, ThriftFunction func, List<Object> args, CompletableRpcResponse reply) {

    try {
        final TBase<?, ?> tArgs = func.newArgs(args);
        if (func.isAsync()) {
            invokeAsynchronously(impl, func, tArgs, reply);
        } else if (ctx.eventLoop().inEventLoop()) {
            invokeSynchronously(ctx, impl, func, tArgs, reply);
        } else {
            invokeSynchronously0(ctx, impl, func, tArgs, reply);
        }
    } catch (Throwable t) {
        reply.completeExceptionally(t);
    }
}

private static void invokeSynchronously(
        ServiceRequestContext ctx, Object impl,
        ThriftFunction func, TBase<?, ?> args, CompletableRpcResponse reply) {
    ctx.blockingTaskExecutor().execute(() -> invokeSynchronously0(ctx, impl, func, args, reply));
}

private static void invokeSynchronously0(
        ServiceRequestContext ctx, Object impl,
        ThriftFunction func, TBase<?, ?> args, CompletableRpcResponse reply) {
    ...
}

trustin

Whether to delegate the call to the blocking task executor should be decided by ThriftCallService, not THttpService.

...ft/thrift0.13/src/test/java/com/linecorp/armeria/server/thrift/THttpServiceBlockingTest.java

ChangguHan · 2024-04-24T12:57:51Z

@trustin
I fix to decide whether to check useBlockingTaskExecutor in ThriftCallService instead of THttpService.
Would you check this?

armeria/thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallService.java

Lines 125 to 139 in 0963905

    
           if (func.isAsync()) { 
        
               if (useBlockingTaskExecutor) { 
        
                   ctx.blockingTaskExecutor().execute(() -> { 
        
                       try { 
        
                           invokeAsynchronously(impl, func, tArgs, reply); 
        
                       } catch (Throwable t) { 
        
                           reply.completeExceptionally(t); 
        
                       } 
        
                   }); 
        
               } else { 
        
                   invokeAsynchronously(impl, func, tArgs, reply); 
        
               } 
        
           } else { 
        
               invokeSynchronously(ctx, impl, func, tArgs, reply); 
        
           }

ChangguHan · 2024-04-24T12:58:48Z

Oops, I realized that we also need to fix ThriftCallService not to use blocking task executor twice:

armeria/thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallService.java

Line 186 in 10424c7

ctx.blockingTaskExecutor().execute(() -> {

private static void invoke(
        ServiceRequestContext ctx,
        Object impl, ThriftFunction func, List<Object> args, CompletableRpcResponse reply) {

    try {
        final TBase<?, ?> tArgs = func.newArgs(args);
        if (func.isAsync()) {
            invokeAsynchronously(impl, func, tArgs, reply);
        } else if (ctx.eventLoop().inEventLoop()) {
            invokeSynchronously(ctx, impl, func, tArgs, reply);
        } else {
            invokeSynchronously0(ctx, impl, func, tArgs, reply);
        }
    } catch (Throwable t) {
        reply.completeExceptionally(t);
    }
}

private static void invokeSynchronously(
        ServiceRequestContext ctx, Object impl,
        ThriftFunction func, TBase<?, ?> args, CompletableRpcResponse reply) {
    ctx.blockingTaskExecutor().execute(() -> invokeSynchronously0(ctx, impl, func, args, reply));
}

private static void invokeSynchronously0(
        ServiceRequestContext ctx, Object impl,
        ThriftFunction func, TBase<?, ?> args, CompletableRpcResponse reply) {
    ...
}

@minwoox Thank you for your review.
I fix to check in ThriftCallService!

thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallService.java

...ft/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallServiceBuilder.java

jrhee17

Looks almost done once @trustin 's comments are addressed 👍

...ft/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallServiceBuilder.java

jrhee17 · 2024-05-08T00:02:52Z

...ft/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallServiceBuilder.java

+     */
+    public ThriftCallServiceBuilder addService(String key, Object implementation) {
+        requireNonNull(implementation, "implementation");
+        this.implementations.put(key, ImmutableList.of(implementation));


Suggested change

this.implementations.put(key, ImmutableList.of(implementation));

implementations.put(key, ImmutableList.of(implementation));

thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallService.java

...ft/thrift0.13/src/test/java/com/linecorp/armeria/server/thrift/THttpServiceBlockingTest.java

minwoox

Looks good! Left minor comments. 👍

thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallService.java

...ft/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallServiceBuilder.java

thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallService.java

...ft/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallServiceBuilder.java

thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/thrift/ThriftDynamicTimeoutTest.java

thrift/thrift0.13/src/test/java/com/linecorp/armeria/it/thrift/ThrottlingRpcServiceTest.java

thrift/thrift0.13/src/test/java/com/linecorp/armeria/server/thrift/THttpClientTest.java

trustin · 2024-06-05T17:23:17Z

...hrift0.13/src/test/java/com/linecorp/armeria/server/thrift/ThriftCallServiceBuilderTest.java

+        assertTrue(service.entries().containsKey(""));
+        final Iterator<?> defaultIterator = service.entries().get("").implementations.iterator();
+        assertEquals(defaultServiceImpl, defaultIterator.next());
+        assertFalse(defaultIterator.hasNext());


I think the assertion logic of this test method is not easy to read. How about building a map that contains the expected keys and values and just compare it to the return value of service.entries()? e.g.

final Map<String, List<Object>> actualEntries = service.entries().entrySet().stream()....collect(...); final Map<String, List<Object>> expectedEntries = ImmutableMap.builder()...build(); assertThat(actualEntries).isEqualsTo(expectedEntries);

I added it.
Would you check this?

jrhee17

Changes look good to me once the build reliably passes 👍 👍 👍

jrhee17 · 2024-06-14T09:17:15Z

...ft/thrift0.13/src/test/java/com/linecorp/armeria/server/thrift/THttpServiceBlockingTest.java

+            resultHandler.onComplete(name);
+            blocking.set(Thread.currentThread().getName().startsWith(BLOCKING_EXECUTOR_PREFIX));


nit; swapping these two may help with flakiness. Otherwise, the response may be sent back and the main test thread may proceed before the blocking value is set

Suggested change

resultHandler.onComplete(name);

blocking.set(Thread.currentThread().getName().startsWith(BLOCKING_EXECUTOR_PREFIX));

blocking.set(Thread.currentThread().getName().startsWith(BLOCKING_EXECUTOR_PREFIX));

resultHandler.onComplete(name);

@jrhee17 Thank you for your comment. I applied it.
By the way, how can I know which test is failed from failed build?
I cannot find the failed tests from CI/CD

@ChangguHan You can check the build scan reports here: #5619 (comment) which are updated when builds are complete. Open the report and go to the 'Tests' section to browse the test result report.

For example, there's only one failed job and it failed due to a flaky test which seems irrelevant to your work, so it's all good! 🟢 https://ge.armeria.dev/s/yq5kpsjeqryju/tests/overview

...ft/thrift0.13/src/test/java/com/linecorp/armeria/server/thrift/THttpServiceBlockingTest.java

ikhoon

Thanks, @ChangguHan! 🙇‍♂️👍

trustin · 2024-06-20T13:49:28Z

...hrift0.13/src/test/java/com/linecorp/armeria/server/thrift/ThriftCallServiceBuilderTest.java

+                "barMap", ImmutableList.of(barServiceImpl),
+                "fooIterableMap", ImmutableList.of(fooServiceImpl, barServiceImpl));
+
+        assertThat(actualEntries).isEqualTo(expectedEntries);


Thanks a lot for cleaning this up!

trustin

Great job, @ChangguHan! 🙇

trustin · 2024-06-20T13:56:22Z

Revised and cleaned up the PR description and commit message.

injae-kim · 2024-06-24T08:54:23Z

Nice work @ChangguHan !!! 👍👍

[Issue-4917] Add blockingTaskExecutor in Thrift services

02c82e0

github-actions bot requested review from ikhoon, jrhee17, minwoox and trustin April 18, 2024 05:50

[Issue-4917] Fix test

3dda165

injae-kim reviewed Apr 19, 2024

View reviewed changes

ikhoon added new feature sprint Issues for OSS Sprint participants labels Apr 22, 2024

ikhoon reviewed Apr 22, 2024

View reviewed changes

[Issue-4917] Add test for Iface

6649eee

minwoox reviewed Apr 23, 2024

View reviewed changes

ChangguHan added 2 commits April 23, 2024 12:00

[Issue-4917] Refactor THttpService.invoke

286f804

[Issue-4917] Fix style

16e9e59

minwoox approved these changes Apr 23, 2024

View reviewed changes

trustin requested changes Apr 23, 2024

View reviewed changes

...ft/thrift0.13/src/test/java/com/linecorp/armeria/server/thrift/THttpServiceBlockingTest.java Outdated Show resolved Hide resolved

ChangguHan added 2 commits April 24, 2024 14:34

[Issue-4917] Run blocking in ThriftCallService

d625b8f

[Issue-4917] add ThriftCallServiceBuilder

0963905

trustin reviewed Apr 30, 2024

View reviewed changes

[Issue-4917] fix to keep compatibility

aa97734

ChangguHan commented May 7, 2024

View reviewed changes

...ft/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallServiceBuilder.java Outdated Show resolved Hide resolved

trustin reviewed May 7, 2024

View reviewed changes

...ft/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallServiceBuilder.java Outdated Show resolved Hide resolved

...ft/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/ThriftCallServiceBuilder.java Outdated Show resolved Hide resolved

jrhee17 reviewed May 8, 2024

View reviewed changes

[Issue-4917] fix to keep compatibility

649bf67

ChangguHan requested a review from trustin May 17, 2024 06:59

ChangguHan requested a review from jrhee17 May 17, 2024 06:59

ChangguHan added 3 commits May 17, 2024 19:06

[Issue-4917] fix to disallowed class(es) in the public API

b3dd89f

[Issue-4917] fix to checkstyle

78ff4d1

[Issue-4917] fix doc

abfc035

minwoox reviewed May 20, 2024

View reviewed changes

ChangguHan and others added 2 commits May 27, 2024 09:32

[Issue-4917] fix docs and add @UnstableApi

c072de7

Merge branch 'main' into feature/issue-4917

bc11840

trustin reviewed Jun 5, 2024

View reviewed changes

trustin and others added 2 commits June 13, 2024 18:32

[Issue-4917] Apply reviews

41723fa

[Issue-4917] Apply reviews

0b94831

ChangguHan requested a review from trustin June 13, 2024 09:38

ChangguHan added 2 commits June 14, 2024 09:49

Merge branch 'main' into feature/issue-4917

d4ad8b6

Merge branch 'main' into feature/issue-4917

23169b0

jrhee17 approved these changes Jun 14, 2024

View reviewed changes

[Issue-4917] Apply reviews

7876ae5

ikhoon approved these changes Jun 17, 2024

View reviewed changes

trustin reviewed Jun 20, 2024

View reviewed changes

trustin approved these changes Jun 20, 2024

View reviewed changes

trustin changed the title ~~[Issue-4917] Add blockingTaskExecutor in Thrift services~~ Provide a way to invoke Thrift service impls from BlockingTaskExecutor Jun 20, 2024

trustin merged commit 52114ed into line:main Jun 20, 2024
14 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a way to invoke Thrift service impls from `BlockingTaskExecutor` #5619

Provide a way to invoke Thrift service impls from `BlockingTaskExecutor` #5619

ChangguHan commented Apr 18, 2024 •

edited by trustin

Loading

CLAassistant commented Apr 18, 2024 •

edited

Loading

github-actions bot commented Apr 18, 2024 •

edited

Loading

injae-kim Apr 19, 2024

ikhoon Apr 22, 2024

ChangguHan Apr 23, 2024 •

edited

Loading

minwoox left a comment

minwoox left a comment

minwoox commented Apr 23, 2024

trustin left a comment

ChangguHan commented Apr 24, 2024

ChangguHan commented Apr 24, 2024

jrhee17 left a comment

jrhee17 May 8, 2024

minwoox left a comment

trustin Jun 5, 2024

ChangguHan Jun 13, 2024

jrhee17 left a comment

jrhee17 Jun 14, 2024

ChangguHan Jun 17, 2024

trustin Jun 20, 2024

trustin Jun 20, 2024

ikhoon left a comment

trustin Jun 20, 2024

trustin left a comment

trustin commented Jun 20, 2024

injae-kim commented Jun 24, 2024

	this.implementations.put(key, ImmutableList.of(implementation));
	implementations.put(key, ImmutableList.of(implementation));

		resultHandler.onComplete(name);
		blocking.set(Thread.currentThread().getName().startsWith(BLOCKING_EXECUTOR_PREFIX));

Provide a way to invoke Thrift service impls from BlockingTaskExecutor #5619

Provide a way to invoke Thrift service impls from BlockingTaskExecutor #5619

Conversation

ChangguHan commented Apr 18, 2024 • edited by trustin Loading

CLAassistant commented Apr 18, 2024 • edited Loading

github-actions bot commented Apr 18, 2024 • edited Loading

🔍 Build Scan® (commit: 7876ae5)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChangguHan Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

minwoox commented Apr 23, 2024

trustin left a comment

Choose a reason for hiding this comment

ChangguHan commented Apr 24, 2024

ChangguHan commented Apr 24, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this 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

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

trustin left a comment

Choose a reason for hiding this comment

trustin commented Jun 20, 2024

injae-kim commented Jun 24, 2024

Provide a way to invoke Thrift service impls from `BlockingTaskExecutor` #5619

Provide a way to invoke Thrift service impls from `BlockingTaskExecutor` #5619

ChangguHan commented Apr 18, 2024 •

edited by trustin

Loading

CLAassistant commented Apr 18, 2024 •

edited

Loading

github-actions bot commented Apr 18, 2024 •

edited

Loading

🔍 Build Scan® (commit: `7876ae5`)

ChangguHan Apr 23, 2024 •

edited

Loading