junkaixue commented on code in PR #2883:
URL: https://github.com/apache/helix/pull/2883#discussion_r1725761113
##########
helix-gateway/src/main/java/org/apache/helix/gateway/HelixGatewayMain.java:
##########
@@ -19,35 +19,32 @@
* under the License.
*/
-import io.grpc.Server;
import java.io.IOException;
-import java.util.concurrent.TimeUnit;
-import org.apache.helix.gateway.channel.HelixGatewayServiceGrpcService;
import org.apache.helix.gateway.service.GatewayServiceManager;
-import org.apache.helix.gateway.util.HelixGatewayGrpcServerBuilder;
+import org.apache.helix.gateway.channel.GatewayServiceChannelConfig;
+
+import static java.lang.Integer.*;
/**
* Main class for Helix Gateway.
* It starts the Helix Gateway grpc service.
+ * args0:zk address
+ * args1: helix gateway groc server port
Review Comment:
NIT: spaces bewteen :.
##########
helix-gateway/src/main/java/org/apache/helix/gateway/channel/HelixGatewayServiceGrpcService.java:
##########
@@ -176,4 +186,37 @@ private void updateObserver(String instanceName, String
clusterName,
_reversedObserverMap.put(streamObserver, new
ImmutablePair<>(instanceName, clusterName));
});
}
+
+ @Override
+ public void start() throws IOException {
+ ServerBuilder serverBuilder =
ServerBuilder.forPort(_config.getGrpcServerPort())
+ .addService(this)
+ .keepAliveTime(_config.getServerHeartBeatInterval(),
+ TimeUnit.SECONDS) // HeartBeat time
+ .keepAliveTimeout(_config.getClientTimeout(),
+ TimeUnit.SECONDS) // KeepAlive client timeout
+ .permitKeepAliveTime(_config.getMaxAllowedClientHeartBeatInterval(),
+ TimeUnit.SECONDS) // Permit min HeartBeat time
+ .permitKeepAliveWithoutCalls(true); // Allow KeepAlive forever
without active RPC
+ if (_config.getEnableReflectionService()) {
+ serverBuilder =
serverBuilder.addService(io.grpc.protobuf.services.ProtoReflectionService.newInstance());
+ }
+ _server = serverBuilder.build();
+
+ logger.info("Starting grpc server now....");
Review Comment:
add more server info? for example with port, and other configs?
##########
helix-gateway/src/main/java/org/apache/helix/gateway/channel/GatewayServiceChannelConfig.java:
##########
@@ -0,0 +1,206 @@
+package org.apache.helix.gateway.channel;
+
+/*
+ * 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.
+ */
+
+
+import static
org.apache.helix.gateway.api.constant.GatewayServiceConfigConstant.*;
+
+
+public class GatewayServiceChannelConfig {
+ // Mode to get helix participant information (inbound information). This
included health check and shard state transition response
+ // We do not support hybrid mode as of now, (i.e. have push mode for
participant liveness detection and pull mode for shard state)
+ public enum ChannelMode {
+ PUSH_MODE, // The gateway service passively receives participant
information
+ POLL_MODE // The gateway service actively polls participant information
+ }
+
+ // NOTE:
+ // For outbound information - stateTransition request, Gateway service will
always push the state transition message.
+ // We do not support participant poll mode for stateTransition request as of
now.
+
+ // channel type for the following 3 information - participant liveness
detection, shard state transition request and response
+ // By default, they are all grpc server, user could define them separately.
+ public enum ChannelType {
+ GRPC_SERVER,
+ GRPC_CLIENT,
+ FILE_SHARE
Review Comment:
Just call it file? SHARE is confusing.
##########
helix-gateway/src/test/java/org/apache/helix/gateway/chanel/GatewayServiceChannelConfigTest.java:
##########
@@ -0,0 +1,71 @@
+package org.apache.helix.gateway.chanel;
+
+/*
+ * 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.
+ */
+
+import org.apache.helix.gateway.channel.GatewayServiceChannelConfig;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class GatewayServiceChannelConfigTest {
+
+ @Test
+ public void testGatewayServiceChannelConfigBuilder() {
+ GatewayServiceChannelConfig.GatewayServiceProcessorConfigBuilder builder =
+ new GatewayServiceChannelConfig.GatewayServiceProcessorConfigBuilder();
+
+ builder.setChannelMode(GatewayServiceChannelConfig.ChannelMode.PUSH_MODE)
+
.setParticipantConnectionChannelType(GatewayServiceChannelConfig.ChannelType.GRPC_SERVER)
+
.setShardStateProcessorType(GatewayServiceChannelConfig.ChannelType.GRPC_SERVER)
+ .setGrpcServerPort(50051)
+ .setServerHeartBeatInterval(30)
+ .setMaxAllowedClientHeartBeatInterval(60)
+ .setClientTimeout(120)
+ .setEnableReflectionService(true)
+ .setPollIntervalSec(10);
+
+ GatewayServiceChannelConfig config = builder.build();
+
+ Assert.assertEquals(config.getChannelMode(),
GatewayServiceChannelConfig.ChannelMode.PUSH_MODE);
+ Assert.assertEquals(config.getParticipantConnectionChannelType(),
GatewayServiceChannelConfig.ChannelType.GRPC_SERVER);
+ Assert.assertEquals(config.getShardStateChannelType(),
GatewayServiceChannelConfig.ChannelType.GRPC_SERVER);
+ Assert.assertEquals(config.getGrpcServerPort(), 50051);
+ Assert.assertEquals(config.getServerHeartBeatInterval(), 30);
+ Assert.assertEquals(config.getMaxAllowedClientHeartBeatInterval(), 60);
+ Assert.assertEquals(config.getClientTimeout(), 120);
+ Assert.assertTrue(config.getEnableReflectionService());
+ Assert.assertEquals(config.getPollIntervalSec(), 10);
+ }
+
+ @Test
+ public void testInvalidConfig(){
+ GatewayServiceChannelConfig.GatewayServiceProcessorConfigBuilder builder =
+ new GatewayServiceChannelConfig.GatewayServiceProcessorConfigBuilder();
+
+
builder.setParticipantConnectionChannelType(GatewayServiceChannelConfig.ChannelType.GRPC_SERVER);
+
+ // assert er get an exception
+ try {
+ builder.build();
+ Assert.fail("Should have thrown an exception");
+ } catch (IllegalArgumentException e) {
+ // expected
+ }
+ }
+}
Review Comment:
NIT: new line.
##########
helix-gateway/src/main/java/org/apache/helix/gateway/channel/HelixGatewayServiceChannelFactory.java:
##########
@@ -0,0 +1,43 @@
+package org.apache.helix.gateway.channel;
+
+/*
+ * 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.
+ */
+
+import org.apache.commons.lang3.NotImplementedException;
+import org.apache.helix.gateway.api.service.HelixGatewayServiceChannel;
+import org.apache.helix.gateway.service.GatewayServiceManager;
+
+
+public class HelixGatewayServiceChannelFactory {
+
+ public static HelixGatewayServiceChannel
createServiceChannel(GatewayServiceChannelConfig config,
+ GatewayServiceManager manager) {
+
+ if (config.getChannelMode() ==
GatewayServiceChannelConfig.ChannelMode.PUSH_MODE) {
+ if (config.getParticipantConnectionChannelType() ==
GatewayServiceChannelConfig.ChannelType.GRPC_SERVER) {
+ return new HelixGatewayServiceGrpcService(manager, config);
+ } else {
+ return new HelixGatewayServicePollModeChannel(config);
Review Comment:
NO need for else clause.
--
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: reviews-unsubscr...@helix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@helix.apache.org
For additional commands, e-mail: reviews-h...@helix.apache.org
