You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "CRZbulabula (via GitHub)" <gi...@apache.org> on 2026/05/20 03:00:38 UTC
[PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
CRZbulabula opened a new pull request, #1466:
URL: https://github.com/apache/ratis/pull/1466
## Summary
Fixes [RATIS-2529](https://issues.apache.org/jira/browse/RATIS-2529): gRPC worker threads permanently inflate to ``availableProcessors * 2`` after follower restart catch-up.
- Add ``raft.grpc.server.worker.event-loop.threads`` and ``raft.grpc.client.worker.event-loop.threads`` (default ``0`` = current gRPC behavior).
- When ``> 0``, build a dedicated ``EpollEventLoopGroup`` (or ``NioEventLoopGroup`` when Epoll is unavailable) of that size and wire it into both the server ``NettyServerBuilder``s and the client / server-to-server ``NettyChannelBuilder``s.
- A single boss + worker group is shared across the admin / client / server ``NettyServerBuilder``s and the ``GrpcServerProtocolClient`` instances managed by one ``GrpcServicesImpl``; both groups are shut down in ``closeImpl()``.
This lets operators cap the worker thread count (e.g. 4–8) so a follower catch-up burst can't permanently expand the shared gRPC default ``EventLoopGroup`` (which never shrinks its threads once started).
## Test plan
- [x] New unit test ``TestGrpcEventLoops`` covers the helper (thread count, channel-type detection, config-key roundtrip, null shutdown).
- [x] New integration test ``TestGrpcWorkerEventLoopThreads`` brings up a 3-node ``MiniRaftClusterWithGrpc`` with capped server (2) and client (1) worker threads and asserts a client request succeeds.
- [x] Existing ``TestCustomGrpcServices``, ``TestGrpcFactory``, ``TestLeaderInstallSnapshotWithGrpc``, ``TestLinearizableReadWithGrpc``, ``TestGroupInfoWithGrpc`` all pass.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on code in PR #1466:
URL: https://github.com/apache/ratis/pull/1466#discussion_r3315322726
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientRpc.java:
##########
@@ -60,15 +62,36 @@ public class GrpcClientRpc extends RaftClientRpcWithProxy<GrpcClientProtocolClie
private final int maxMessageSize;
private final TimeDuration requestTimeoutDuration;
private final TimeDuration watchRequestTimeoutDuration;
+ private final EventLoopGroup clientWorkers;
Review Comment:
Done in 00bd24ef. `GrpcClientRpc` now stores the client workers as a `MemoizedSupplier<EventLoopGroup>`, and `GrpcClientProtocolClient` obtains the group lazily when building a channel.
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientRpc.java:
##########
@@ -213,6 +236,15 @@ private RaftClientRequestProto toRaftClientRequestProto(RaftClientRequest reques
return proto;
}
+ @Override
+ public void close() {
+ try {
+ super.close();
+ } finally {
+ NettyUtils.shutdownGracefully(clientWorkers);
Review Comment:
Done in 00bd24ef. `close()` now checks `clientWorkers.isInitialized()` before shutting down the shared client worker group.
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientRpc.java:
##########
@@ -60,15 +62,36 @@ public class GrpcClientRpc extends RaftClientRpcWithProxy<GrpcClientProtocolClie
private final int maxMessageSize;
private final TimeDuration requestTimeoutDuration;
private final TimeDuration watchRequestTimeoutDuration;
+ private final EventLoopGroup clientWorkers;
public GrpcClientRpc(ClientId clientId, RaftProperties properties,
SslContext adminSslContext, SslContext clientSslContext) {
+ this(clientId, properties, adminSslContext, clientSslContext, newClientWorkers(clientId, properties));
+ }
Review Comment:
Done in 00bd24ef. I added `GrpcClientRpc.create(...)`, made the constructor private, and updated `GrpcFactory` to use the factory method.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1466:
URL: https://github.com/apache/ratis/pull/1466#discussion_r3276156221
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/GrpcConfigKeys.java:
##########
@@ -155,6 +155,22 @@ static GrpcTlsConfig tlsConf(Parameters parameters) {
static void setTlsConf(Parameters parameters, GrpcTlsConfig conf) {
parameters.put(TLS_CONF_PARAMETER, conf, TLS_CONF_CLASS);
}
+
+ /**
+ * The number of worker threads for the gRPC client-side {@link
+ * org.apache.ratis.thirdparty.io.netty.channel.EventLoopGroup}.
+ * 0 (default) means use the gRPC default (i.e. {@code availableProcessors * 2});
+ * a positive value caps the worker event-loop thread count.
+ */
+ String WORKER_EVENT_LOOP_THREADS_KEY = PREFIX + ".worker.event-loop.threads";
Review Comment:
In NettyConfigKeys, we have the following conf:
```
key: raft.netty.dataStream.client.use-epoll (boolean, default=true)
key: raft.netty.dataStream.client.worker-group.size (int, default=28)
```
Let's use the same similar conf for gRPC.
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcServicesImpl.java:
##########
@@ -234,6 +242,24 @@ Server newServer(GrpcClientProtocolService client, ServerInterceptor interceptor
return buildServer(serverBuilder, types);
}
+ private EventLoopGroup bossEventLoopGroup;
+ private EventLoopGroup workerEventLoopGroup;
+
+ EventLoopGroup getBossEventLoopGroup() {
+ if (workerEventLoopThreads > 0 && bossEventLoopGroup == null) {
+ bossEventLoopGroup = GrpcEventLoops.newEventLoopGroup(1, server.getId() + "-grpc-boss-ELG");
Review Comment:
Let's make bossEventLoopThreads configurable?
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcServicesImpl.java:
##########
@@ -274,20 +300,27 @@ public static Builder newBuilder() {
private final GrpcClientProtocolService clientProtocolService;
private final MetricServerInterceptor serverInterceptor;
+ private final EventLoopGroup bossEventLoopGroup;
+ private final EventLoopGroup workerEventLoopGroup;
private GrpcServicesImpl(Builder b) {
- super(b.server::getId, id -> new PeerProxyMap<>(id.toString(), b::newGrpcServerProtocolClient));
+ super(b.server::getId, id -> new PeerProxyMap<>(id.toString(),
+ peer -> b.newGrpcServerProtocolClient(peer, b.getWorkerEventLoopGroup())));
Review Comment:
We should create only one group for all clients.
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcServicesImpl.java:
##########
@@ -375,6 +408,9 @@ public void closeImpl() {
} catch (IOException e) {
LOG.warn("{}: Failed to close proxies", getId(), e);
}
+
+ GrpcEventLoops.shutdownGracefully(workerEventLoopGroup);
+ GrpcEventLoops.shutdownGracefully(bossEventLoopGroup);
Review Comment:
We should call shutdownGracefully() for all groups and then await(..)
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcServicesImpl.java:
##########
@@ -274,20 +300,27 @@ public static Builder newBuilder() {
private final GrpcClientProtocolService clientProtocolService;
private final MetricServerInterceptor serverInterceptor;
+ private final EventLoopGroup bossEventLoopGroup;
+ private final EventLoopGroup workerEventLoopGroup;
Review Comment:
We also need a group for client. BTW, let's use shorter names.
```java
private EventLoopGroup serverBosses;
private EventLoopGroup serverWorkers;
private EventLoopGroup clientWorkers;
```
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/GrpcEventLoops.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.ratis.grpc;
+
+import org.apache.ratis.thirdparty.io.grpc.netty.NettyChannelBuilder;
+import org.apache.ratis.thirdparty.io.grpc.netty.NettyServerBuilder;
+import org.apache.ratis.thirdparty.io.netty.channel.Channel;
+import org.apache.ratis.thirdparty.io.netty.channel.EventLoopGroup;
+import org.apache.ratis.thirdparty.io.netty.channel.ServerChannel;
+import org.apache.ratis.thirdparty.io.netty.channel.epoll.Epoll;
+import org.apache.ratis.thirdparty.io.netty.channel.epoll.EpollEventLoopGroup;
+import org.apache.ratis.thirdparty.io.netty.channel.epoll.EpollServerSocketChannel;
+import org.apache.ratis.thirdparty.io.netty.channel.epoll.EpollSocketChannel;
+import org.apache.ratis.thirdparty.io.netty.channel.nio.NioEventLoopGroup;
+import org.apache.ratis.thirdparty.io.netty.channel.socket.nio.NioServerSocketChannel;
+import org.apache.ratis.thirdparty.io.netty.channel.socket.nio.NioSocketChannel;
+import org.apache.ratis.thirdparty.io.netty.util.concurrent.DefaultThreadFactory;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Helper for creating bounded Netty {@link EventLoopGroup} instances for gRPC,
+ * along with matching {@link Channel} / {@link ServerChannel} types.
+ *
+ * <p>By default, gRPC uses a shared {@link EventLoopGroup} sized to
+ * {@code Runtime.getRuntime().availableProcessors() * 2}, and Netty threads
+ * in that group never exit once started. A traffic burst (e.g. a follower
+ * catch-up after restart) can permanently inflate the active thread count.
+ * This helper lets callers construct a smaller, dedicated group and wire it
+ * into {@link NettyServerBuilder} / {@link NettyChannelBuilder} consistently
+ * with the matching channel type.
+ */
+public final class GrpcEventLoops {
Review Comment:
Let's move NettyUtils to ratis-common and then add the new methods there.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1466:
URL: https://github.com/apache/ratis/pull/1466#issuecomment-4536214302
@CRZbulabula , thanks for the update! The change looks good.
However, there are some test failures (attempted twice). I am not sure if it is related. It may be the cases that it uses too many threads in some tests and causing timeout. Let's try rerunning the failing tests one more time.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on code in PR #1466:
URL: https://github.com/apache/ratis/pull/1466#discussion_r3273647493
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/GrpcConfigKeys.java:
##########
@@ -291,6 +307,23 @@ static int stubPoolSize(RaftProperties properties) {
static void setStubPoolSize(RaftProperties properties, int size) {
setInt(properties::setInt, STUB_POOL_SIZE_KEY, size);
}
+
+ /**
+ * The number of worker threads for the gRPC server-side {@link
+ * org.apache.ratis.thirdparty.io.netty.channel.EventLoopGroup}.
+ * The group is also reused by the server-to-server clients managed by this server.
+ * 0 (default) means use the gRPC default (i.e. {@code availableProcessors * 2});
+ * a positive value caps the worker event-loop thread count.
+ */
+ String WORKER_EVENT_LOOP_THREADS_KEY = PREFIX + ".worker.event-loop.threads";
+ int WORKER_EVENT_LOOP_THREADS_DEFAULT = 0;
Review Comment:
we need some positive number here like 16/24/32 or some function from CPU cores number.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "yandrey321 (via GitHub)" <gi...@apache.org>.
yandrey321 commented on code in PR #1466:
URL: https://github.com/apache/ratis/pull/1466#discussion_r3273644410
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/GrpcConfigKeys.java:
##########
@@ -155,6 +155,22 @@ static GrpcTlsConfig tlsConf(Parameters parameters) {
static void setTlsConf(Parameters parameters, GrpcTlsConfig conf) {
parameters.put(TLS_CONF_PARAMETER, conf, TLS_CONF_CLASS);
}
+
+ /**
+ * The number of worker threads for the gRPC client-side {@link
+ * org.apache.ratis.thirdparty.io.netty.channel.EventLoopGroup}.
+ * 0 (default) means use the gRPC default (i.e. {@code availableProcessors * 2});
+ * a positive value caps the worker event-loop thread count.
+ */
+ String WORKER_EVENT_LOOP_THREADS_KEY = PREFIX + ".worker.event-loop.threads";
+ int WORKER_EVENT_LOOP_THREADS_DEFAULT = 0;
Review Comment:
we need some positive number here like 16/24/32 or some function from CPU cores number.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1466:
URL: https://github.com/apache/ratis/pull/1466#discussion_r3319558802
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcServicesImpl.java:
##########
@@ -235,7 +243,26 @@ Server newServer(GrpcClientProtocolService client, ServerInterceptor interceptor
}
public GrpcServicesImpl build() {
- return new GrpcServicesImpl(this);
+ final RaftProperties props = server.getProperties();
+ final String id = server.getId() + "";
+ final boolean useEpoll = GrpcConfigKeys.useEpoll(props);
+ try {
+ serverBosses = NettyUtils.newEventLoopGroup(id + "-boss",
+ GrpcConfigKeys.Server.bossGroupSize(props), useEpoll);
+ serverWorkers = NettyUtils.newEventLoopGroup(id + "-server-workers",
+ GrpcConfigKeys.Server.workerGroupSize(props), useEpoll);
+ clientWorkers = NettyUtils.newEventLoopGroup(id + "-client-workers",
+ GrpcConfigKeys.Client.workerGroupSize(props), useEpoll);
+ return new GrpcServicesImpl(this);
+ } catch (Throwable t) {
+ NettyUtils.shutdownGracefully(clientWorkers, serverWorkers, serverBosses);
+ if (t instanceof RuntimeException) {
+ throw (RuntimeException) t;
+ } else if (t instanceof Error) {
+ throw (Error) t;
+ }
+ throw new RuntimeException(t);
Review Comment:
We could just `throw t`. Java can take care the casting.
```java
} catch (Throwable t) {
NettyUtils.shutdownGracefully(clientWorkers, serverWorkers, serverBosses);
throw t;
}
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1466:
URL: https://github.com/apache/ratis/pull/1466#issuecomment-4587189648
@CRZbulabula , the change looks good. Could you resolve the conflicts with RATIS-2548?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on PR #1466:
URL: https://github.com/apache/ratis/pull/1466#issuecomment-4585946369
Rebased this PR on current `master` and pushed `f33b8ebb`.
During the rebase, the ReplyFlusher conflict resolved to the upstream RATIS-2547 implementation already merged in #1474, so I kept this PR scoped to the gRPC EventLoopGroup / NettyUtils changes.
Additional fix after CI:
- Stabilized `TestRaftServerWithGrpc#testRaftServerMetrics` by waiting for the first async reply batch to finish after unblocking the state machine flush, before sending the oversized request used by the request-size metric assertion.
Local verification:
- `mvn -pl ratis-test -am -Dtest=TestRaftServerWithGrpc#testRaftServerMetrics test`
- `mvn -pl ratis-test -am -Dtest=TestRaftServerWithGrpc test`
GitHub Actions for `f33b8ebb` are green; coverage is skipped as expected.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on PR #1466:
URL: https://github.com/apache/ratis/pull/1466#issuecomment-4571093498
Updated this PR after the latest review:
* Rebased the branch on current `apache/ratis:master`.
* Kept this PR scoped to the gRPC EventLoopGroup / NettyUtils changes.
* Applied the latest `throw t` cleanup in `GrpcServicesImpl.Builder#build()`.
* Split the unrelated fixes into separate PRs:
* RATIS-2547: https://github.com/apache/ratis/pull/1474
* RATIS-2548: https://github.com/apache/ratis/pull/1475
Local verification for this cleaned branch:
`mvn -pl ratis-grpc,ratis-netty,ratis-test -am -DskipTests clean test-compile`
`mvn -pl ratis-test -am -Dtest=TestGrpcEventLoops,TestGrpcWorkerEventLoopThreads,TestGrpcClientEventLoops test`
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1466:
URL: https://github.com/apache/ratis/pull/1466
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1466:
URL: https://github.com/apache/ratis/pull/1466#discussion_r3306065120
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcServicesImpl.java:
##########
@@ -235,7 +243,21 @@ Server newServer(GrpcClientProtocolService client, ServerInterceptor interceptor
}
public GrpcServicesImpl build() {
- return new GrpcServicesImpl(this);
+ final RaftProperties props = server.getProperties();
+ final String id = server.getId() + "";
+ final boolean useEpoll = GrpcConfigKeys.useEpoll(props);
+ try {
+ serverBosses = NettyUtils.newEventLoopGroup(id + "-boss",
+ GrpcConfigKeys.Server.bossGroupSize(props), useEpoll);
+ serverWorkers = NettyUtils.newEventLoopGroup(id + "-server-workers",
+ GrpcConfigKeys.Server.workerGroupSize(props), useEpoll);
+ clientWorkers = NettyUtils.newEventLoopGroup(id + "-client-workers",
+ GrpcConfigKeys.Client.workerGroupSize(props), useEpoll);
+ return new GrpcServicesImpl(this);
+ } catch (RuntimeException | Error e) {
Review Comment:
Let's catch Throwable?
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientRpc.java:
##########
@@ -213,6 +236,15 @@ private RaftClientRequestProto toRaftClientRequestProto(RaftClientRequest reques
return proto;
}
+ @Override
+ public void close() {
+ try {
+ super.close();
+ } finally {
+ NettyUtils.shutdownGracefully(clientWorkers);
Review Comment:
Check if it is initialized:
```java
if (clientWorkers.isInitialized()) {
NettyUtils.shutdownGracefully(clientWorkers.get());
}
```
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcServerProtocolClient.java:
##########
@@ -90,6 +96,10 @@ private ManagedChannel buildChannel(RaftPeer target, int flowControlWindow, SslC
channelBuilder.negotiationType(NegotiationType.PLAINTEXT);
}
channelBuilder.disableRetry();
+ if (eventLoopGroup != null) {
Review Comment:
It won't be null. Let's add requireNonNull:
```java
this.eventLoopGroup = Objects.requireNonNull(eventLoopGroup, "eventLoopGroup == null");
```
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcStubPool.java:
##########
@@ -53,6 +55,10 @@ static ManagedChannel buildManagedChannel(String address, SslContext sslContext)
} else {
channelBuilder.negotiationType(NegotiationType.PLAINTEXT);
}
+ if (eventLoopGroup != null) {
Review Comment:
Similarly, it won't be null.
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientRpc.java:
##########
@@ -60,15 +62,36 @@ public class GrpcClientRpc extends RaftClientRpcWithProxy<GrpcClientProtocolClie
private final int maxMessageSize;
private final TimeDuration requestTimeoutDuration;
private final TimeDuration watchRequestTimeoutDuration;
+ private final EventLoopGroup clientWorkers;
public GrpcClientRpc(ClientId clientId, RaftProperties properties,
SslContext adminSslContext, SslContext clientSslContext) {
+ this(clientId, properties, adminSslContext, clientSslContext, newClientWorkers(clientId, properties));
+ }
Review Comment:
Let's change it to a static method:
```java
public static GrpcClientRpc create(ClientId clientId, RaftProperties properties,
SslContext adminSslContext, SslContext clientSslContext) {
final MemoizedSupplier<EventLoopGroup> eventLoopGroup = MemoizedSupplier.valueOf(() -> NettyUtils.newEventLoopGroup(
clientId + "-client-workers",
GrpcConfigKeys.Client.workerGroupSize(properties),
GrpcConfigKeys.useEpoll(properties)));
return new GrpcClientRpc(clientId, properties, adminSslContext, clientSslContext, eventLoopGroup);
}
```
```java
private final MemoizedSupplier<EventLoopGroup> clientWorkers;
private GrpcClientRpc(ClientId clientId, RaftProperties properties,
SslContext adminSslContext, SslContext clientSslContext, MemoizedSupplier<EventLoopGroup> clientWorkers) {
super(new PeerProxyMap<>(clientId.toString(),
p -> new GrpcClientProtocolClient(clientId, p, properties, adminSslContext, clientSslContext, clientWorkers)));
this.clientWorkers = clientWorkers;
this.clientId = clientId;
this.maxMessageSize = GrpcConfigKeys.messageSizeMax(properties, LOG::debug).getSizeInt();
this.requestTimeoutDuration = RaftClientConfigKeys.Rpc.requestTimeout(properties);
this.watchRequestTimeoutDuration = RaftClientConfigKeys.Rpc.watchRequestTimeout(properties);
}
```
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/client/GrpcClientRpc.java:
##########
@@ -60,15 +62,36 @@ public class GrpcClientRpc extends RaftClientRpcWithProxy<GrpcClientProtocolClie
private final int maxMessageSize;
private final TimeDuration requestTimeoutDuration;
private final TimeDuration watchRequestTimeoutDuration;
+ private final EventLoopGroup clientWorkers;
Review Comment:
Let's use a MemoizedSupplier for lazy initialization.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on PR #1466:
URL: https://github.com/apache/ratis/pull/1466#issuecomment-4535184461
Thanks for the review. I have pushed commit 2088cdf7 addressing the comments:
- Moved NettyUtils to ratis-common as org.apache.ratis.util.NettyUtils and removed the separate GrpcEventLoops helper.
- Added gRPC configs similar to NettyConfigKeys:
- raft.grpc.use-epoll
- raft.grpc.client.worker-group.size
- raft.grpc.server.worker-group.size
- raft.grpc.server.boss-group.size
- The worker-group.size defaults are now positive and CPU-based: Math.max(1, NettyRuntime.availableProcessors() * 2).
- GrpcServicesImpl now creates one serverBosses group, one serverWorkers group, and one clientWorkers group. The server builders share serverBosses/serverWorkers, and all server-to-server clients share clientWorkers.
- NettyUtils.shutdownGracefully(...) now starts shutdownGracefully() for all groups first, then awaits termination.
- Verified with: mvn -pl ratis-grpc,ratis-netty,ratis-test -am -DskipTests clean test-compile
For the performance question: I ran a manual MiniRaftClusterWithGrpc benchmark on my macOS arm64 host with JDK 21, using 500 warmup ops and 5000 measured async sends at concurrency 16, with 3 iterations per config. Median results for worker sizes 8/16/24/32 were about 290/285/291/313 ops/s, p50 53.4/56.0/54.3/49.8 ms, p99 75.6/75.4/71.2/77.0 ms, and 0 failures. This did not show a regression from using bounded/configurable worker groups; the gRPC worker group handles non-blocking I/O while request execution is still handled by raft.grpc.server.async.request.thread.pool.size.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1466:
URL: https://github.com/apache/ratis/pull/1466#discussion_r3276156221
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/GrpcConfigKeys.java:
##########
@@ -155,6 +155,22 @@ static GrpcTlsConfig tlsConf(Parameters parameters) {
static void setTlsConf(Parameters parameters, GrpcTlsConfig conf) {
parameters.put(TLS_CONF_PARAMETER, conf, TLS_CONF_CLASS);
}
+
+ /**
+ * The number of worker threads for the gRPC client-side {@link
+ * org.apache.ratis.thirdparty.io.netty.channel.EventLoopGroup}.
+ * 0 (default) means use the gRPC default (i.e. {@code availableProcessors * 2});
+ * a positive value caps the worker event-loop thread count.
+ */
+ String WORKER_EVENT_LOOP_THREADS_KEY = PREFIX + ".worker.event-loop.threads";
Review Comment:
In NettyConfigKeys, we have the following conf:
```
key: raft.netty.dataStream.client.use-epoll (boolean, default=true)
key: raft.netty.dataStream.client.worker-group.size (int, default=28)
```
Let's use similar conf for gRPC.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on code in PR #1466:
URL: https://github.com/apache/ratis/pull/1466#discussion_r3315324316
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcServerProtocolClient.java:
##########
@@ -90,6 +96,10 @@ private ManagedChannel buildChannel(RaftPeer target, int flowControlWindow, SslC
channelBuilder.negotiationType(NegotiationType.PLAINTEXT);
}
channelBuilder.disableRetry();
+ if (eventLoopGroup != null) {
Review Comment:
Done in 00bd24ef. The constructor now uses `Objects.requireNonNull(eventLoopGroup, "eventLoopGroup == null")`, and `buildChannel(...)` always sets the channel type and event-loop group.
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcServicesImpl.java:
##########
@@ -235,7 +243,21 @@ Server newServer(GrpcClientProtocolService client, ServerInterceptor interceptor
}
public GrpcServicesImpl build() {
- return new GrpcServicesImpl(this);
+ final RaftProperties props = server.getProperties();
+ final String id = server.getId() + "";
+ final boolean useEpoll = GrpcConfigKeys.useEpoll(props);
+ try {
+ serverBosses = NettyUtils.newEventLoopGroup(id + "-boss",
+ GrpcConfigKeys.Server.bossGroupSize(props), useEpoll);
+ serverWorkers = NettyUtils.newEventLoopGroup(id + "-server-workers",
+ GrpcConfigKeys.Server.workerGroupSize(props), useEpoll);
+ clientWorkers = NettyUtils.newEventLoopGroup(id + "-client-workers",
+ GrpcConfigKeys.Client.workerGroupSize(props), useEpoll);
+ return new GrpcServicesImpl(this);
+ } catch (RuntimeException | Error e) {
Review Comment:
Done in 00bd24ef. `Builder#build()` now catches `Throwable`, shuts down `clientWorkers`, `serverWorkers`, and `serverBosses`, then rethrows `RuntimeException` and `Error` directly.
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcStubPool.java:
##########
@@ -53,6 +55,10 @@ static ManagedChannel buildManagedChannel(String address, SslContext sslContext)
} else {
channelBuilder.negotiationType(NegotiationType.PLAINTEXT);
}
+ if (eventLoopGroup != null) {
Review Comment:
Done in 00bd24ef. `GrpcStubPool.buildManagedChannel(...)` now requires a non-null event-loop group and always wires it into the Netty channel builder.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2529. Bound gRPC worker EventLoopGroup thread count. [ratis]
Posted by "CRZbulabula (via GitHub)" <gi...@apache.org>.
CRZbulabula commented on PR #1466:
URL: https://github.com/apache/ratis/pull/1466#issuecomment-4560798870
Thanks for the review. I have pushed commit 00bd24ef addressing the latest comments:
- `GrpcClientRpc` now uses a `MemoizedSupplier<EventLoopGroup>` and exposes the suggested static `create(...)` factory.
- The client worker group is initialized lazily and is shut down only when the supplier has been initialized.
- `GrpcServicesImpl.Builder#build()` now catches `Throwable`, shuts down any created event-loop groups, and rethrows `RuntimeException` / `Error` directly.
- `GrpcServerProtocolClient` and `GrpcStubPool` now require a non-null event-loop group and always configure the Netty channel with it.
I also created the JIRAs for the unrelated fixes:
- https://issues.apache.org/jira/browse/RATIS-2547
- https://issues.apache.org/jira/browse/RATIS-2548
What would you prefer as the next step for those unrelated fixes: keep them in this PR with the JIRA references, or split them into separate PRs?
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org