szetszwo commented on a change in pull request #401:
URL: https://github.com/apache/incubator-ratis/pull/401#discussion_r562353312
##########
File path: ratis-grpc/src/main/java/org/apache/ratis/grpc/GrpcFactory.java
##########
@@ -61,17 +64,41 @@ public static Parameters newRaftParameters(GrpcTlsConfig
conf) {
}
public GrpcFactory(Parameters parameters) {
- this(GrpcConfigKeys.TLS.conf(parameters));
+ this(
+ GrpcConfigKeys.Admin.tlsConf(parameters),
+ GrpcConfigKeys.Client.tlsConf(parameters),
+ GrpcConfigKeys.Server.tlsConf(parameters),
+ GrpcConfigKeys.TLS.conf(parameters));
}
public GrpcFactory(GrpcTlsConfig tlsConfig) {
+ this(null, null, null, tlsConfig);
+ }
+
+ private GrpcFactory(GrpcTlsConfig adminTlsConfig, GrpcTlsConfig
clientTlsConfig,
+ GrpcTlsConfig serverTlsConfig, GrpcTlsConfig tlsConfig) {
Review comment:
Let's make tlsConfig to be the first parameter.
##########
File path:
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcService.java
##########
@@ -82,8 +103,10 @@ public static Builder newBuilder() {
return new Builder();
}
- private final Server server;
+ private final Set<Server> servers = new LinkedHashSet<>(3);
Review comment:
The Server class does not override hashCode and equal so that the
behavior may be unexpected. Let's use a prot-to-Server Map instead?
##########
File path: ratis-examples/src/main/bin/common.sh
##########
@@ -38,7 +38,7 @@ fi
echo "Found ${ARTIFACT}"
-QUORUM_OPTS="--peers n0:localhost:6000,n1:localhost:6001,n2:localhost:6002"
+QUORUM_OPTS="--peers
n0:localhost:6000:6010:6020:6030,n1:localhost:6001:6011:6021:6031,n2:localhost:6002:6012:6022:6032"
Review comment:
We should not change the command option here. The port setting should
be optional, i.e. when there is only a single port, it will be used for both
client and admin.
##########
File path: ratis-common/src/main/java/org/apache/ratis/protocol/RaftPeer.java
##########
@@ -141,11 +181,21 @@ public RaftPeerId getId() {
return id;
}
- /** @return The RPC address of the peer. */
+ /** @return The RPC address of the peer for server-server communication. */
public String getAddress() {
return address;
}
+ /** @return The RPC address of the peer for admin operations. */
+ public String getAdminAddress() {
+ return adminAddress != null ? adminAddress : address;
Review comment:
getAdminAddress() should just return adminAddress. The callers should
fallback to use getAddress() when getAdminAddress() returns null.
<img width="932" alt="image"
src="https://user-images.githubusercontent.com/907380/105442392-c9810580-5ca4-11eb-8c1b-0da0d6dfb043.png";>
In the current usages, we already use Optional.ofNullable(..) to handle the
case getAdminAddress() returning null.
##########
File path:
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcService.java
##########
@@ -231,4 +323,40 @@ public StartLeaderElectionReplyProto
startLeaderElection(StartLeaderElectionRequ
final RaftPeerId target =
RaftPeerId.valueOf(request.getServerRequest().getReplyId());
return getProxies().getProxy(target).startLeaderElection(request);
}
+
+ private static final class ServerConfig {
+
+ private final int port;
+ private final GrpcTlsConfig tlsConfig;
+
+ private ServerConfig(int port, GrpcTlsConfig tlsConfig) {
+ this.port = port;
+ this.tlsConfig = tlsConfig;
+ }
+
+ public int getPort() {
+ return port;
+ }
+
+ public GrpcTlsConfig getTlsConfig() {
+ return tlsConfig;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) {
+ return true;
+ }
+ if (o == null || getClass() != o.getClass()) {
+ return false;
+ }
+ ServerConfig that = (ServerConfig) o;
+ return port == that.port && port > 0;
+ }
+
+ @Override
+ public int hashCode() {
+ return Objects.hash(port);
+ }
Review comment:
We may compare getPort() directly as mentioned above. Then, we don't
have to override equals(..) and hashCode() here.
##########
File path: ratis-common/src/main/java/org/apache/ratis/protocol/RaftPeer.java
##########
@@ -141,11 +181,21 @@ public RaftPeerId getId() {
return id;
}
- /** @return The RPC address of the peer. */
+ /** @return The RPC address of the peer for server-server communication. */
public String getAddress() {
return address;
}
+ /** @return The RPC address of the peer for admin operations. */
+ public String getAdminAddress() {
+ return adminAddress != null ? adminAddress : address;
+ }
+
+ /** @return The RPC address of the peer for client operations. */
+ public String getClientAddress() {
+ return clientAddress != null ? clientAddress : address;
Review comment:
Same as getAdminAddress(), getClientAddress() should just return
clientAddress.
##########
File path:
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcService.java
##########
@@ -118,23 +144,76 @@ private GrpcService(RaftServer raftServer,
Supplier<RaftPeerId> idSupplier, int
this.clientProtocolService = new GrpcClientProtocolService(idSupplier,
raftServer);
+ final int port = serverConfig.getPort();
this.serverInterceptor = new MetricServerInterceptor(
idSupplier,
JavaUtils.getClassSimpleName(getClass()) + "_" + port
);
- NettyServerBuilder nettyServerBuilder = NettyServerBuilder.forPort(port)
+ final boolean separateAdminServer =
!serverConfig.equals(adminServerConfig);
+ final boolean separateClientServer =
!serverConfig.equals(clientServerConfig);
Review comment:
Let's compare getPort(), i.e.
```
final boolean separateAdminServer = adminServerConfig.getPort() != port;
final boolean separateClientServer = clientServerConfig.getPort() !=
port;
```
##########
File path:
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcService.java
##########
@@ -118,23 +144,76 @@ private GrpcService(RaftServer raftServer,
Supplier<RaftPeerId> idSupplier, int
this.clientProtocolService = new GrpcClientProtocolService(idSupplier,
raftServer);
+ final int port = serverConfig.getPort();
this.serverInterceptor = new MetricServerInterceptor(
idSupplier,
JavaUtils.getClassSimpleName(getClass()) + "_" + port
);
- NettyServerBuilder nettyServerBuilder = NettyServerBuilder.forPort(port)
+ final boolean separateAdminServer =
!serverConfig.equals(adminServerConfig);
+ final boolean separateClientServer =
!serverConfig.equals(clientServerConfig);
+
+ final NettyServerBuilder serverBuilder =
+ startBuildingNettyServer(serverConfig, grpcMessageSizeMax,
flowControlWindow);
+ serverBuilder.addService(ServerInterceptors.intercept(
+ new GrpcServerProtocolService(idSupplier, raftServer),
serverInterceptor));
+ if (!separateAdminServer) {
+ addAdminService(raftServer, serverBuilder);
+ }
+ if (!separateClientServer) {
+ addClientService(serverBuilder);
Review comment:
Similarly, let's move this line down to the else-clause (line 190) below?
##########
File path:
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcService.java
##########
@@ -118,23 +144,76 @@ private GrpcService(RaftServer raftServer,
Supplier<RaftPeerId> idSupplier, int
this.clientProtocolService = new GrpcClientProtocolService(idSupplier,
raftServer);
+ final int port = serverConfig.getPort();
this.serverInterceptor = new MetricServerInterceptor(
idSupplier,
JavaUtils.getClassSimpleName(getClass()) + "_" + port
);
- NettyServerBuilder nettyServerBuilder = NettyServerBuilder.forPort(port)
+ final boolean separateAdminServer =
!serverConfig.equals(adminServerConfig);
+ final boolean separateClientServer =
!serverConfig.equals(clientServerConfig);
+
+ final NettyServerBuilder serverBuilder =
+ startBuildingNettyServer(serverConfig, grpcMessageSizeMax,
flowControlWindow);
+ serverBuilder.addService(ServerInterceptors.intercept(
+ new GrpcServerProtocolService(idSupplier, raftServer),
serverInterceptor));
+ if (!separateAdminServer) {
+ addAdminService(raftServer, serverBuilder);
Review comment:
Let's move this line down to the else-clause (line 179) below?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
![https://user-images.githubusercontent.com/907380/105442392-c9810580-5ca4-11eb-8c1b-0da0d6dfb043.png"](https://user-images.githubusercontent.com/907380/105442392-c9810580-5ca4-11eb-8c1b-0da0d6dfb043.png"){kind=link}