You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "xyuanlu (via GitHub)" <gi...@apache.org> on 2024/08/09 22:38:40 UTC
[PR] Create Gateway service channel factory [helix]
xyuanlu opened a new pull request, #2883:
URL: https://github.com/apache/helix/pull/2883
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
#2821
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
(Write a concise description including what, why, how)
Create Gateway service channel factory
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Changes that Break Backward Compatibility (Optional)
- My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on PR #2883:
URL: https://github.com/apache/helix/pull/2883#issuecomment-2306365293
This pr is ready to be merged. Approved by @junkaixue
Commit message:
Create Gateway service channel factory
This PR introduces a new configuration system for the Helix Gateway service, allowing for more flexible and customizable channel configurations. The main changes include:
-Introduction of GatewayServiceChannelConfig class to manage various channel configurations.
-Implementation of a factory pattern (HelixGatewayServiceChannelFactory) for creating appropriate service channels based on the configuration.
-The PR also includes various improvements in error handling, logging, and code organization.
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2883:
URL: https://github.com/apache/helix/pull/2883#discussion_r1720376747
##########
helix-gateway/src/main/java/org/apache/helix/gateway/channel/GatewayServiceChannelConfig.java:
##########
@@ -0,0 +1,173 @@
+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.GatewayServiceDefaultConfig.*;
+
+public class GatewayServiceChannelConfig {
+ public enum ChannelType {
+ GRPC_SERVER, POLL_GRPC, SHARED_FILE
Review Comment:
At least, let's make name consistent:
GRPC_PUSH
GRPC_POLL
FILE_POLL
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2883:
URL: https://github.com/apache/helix/pull/2883#discussion_r1725948069
##########
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:
I misplace the initiation on a wrong branch.... Now it need the else
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2883:
URL: https://github.com/apache/helix/pull/2883#discussion_r1724032932
##########
helix-gateway/src/main/java/org/apache/helix/gateway/channel/HelixGatewayServiceChannelFactory.java:
##########
@@ -0,0 +1,42 @@
+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,
Review Comment:
Updated
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2883:
URL: https://github.com/apache/helix/pull/2883#discussion_r1719078940
##########
helix-gateway/src/main/java/org/apache/helix/gateway/HelixGatewayMain.java:
##########
@@ -38,16 +37,11 @@ private HelixGatewayMain() {
public static void main(String[] args) throws InterruptedException, IOException {
// Create a new server to listen on port 50051
- GatewayServiceManager manager = new GatewayServiceManager();
- Server server = new HelixGatewayGrpcServerBuilder().setPort(50051)
- .setGrpcService((HelixGatewayServiceGrpcService)manager.getHelixGatewayServiceProcessor())
- .build();
-
- server.start();
- System.out.println("Server started, listening on " + server.getPort());
+ GatewayServiceChannelConfig.GatewayServiceProcessorConfigBuilder builder = new GatewayServiceChannelConfig.GatewayServiceProcessorConfigBuilder();
+ GatewayServiceManager manager = new GatewayServiceManager(args[0],
+ builder.setGrpcServerPort(50051).build());
Review Comment:
Let's get this as a pass in argument?
##########
helix-gateway/src/main/java/org/apache/helix/gateway/api/constant/GatewayServiceDefaultConfig.java:
##########
@@ -0,0 +1,28 @@
+package org.apache.helix.gateway.api.constant;
+
+/*
+ * 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.
+ */
+
+public class GatewayServiceDefaultConfig {
Review Comment:
Shall we call it constant or keys instead of configs.
##########
helix-gateway/src/main/java/org/apache/helix/gateway/channel/GatewayServiceChannelConfig.java:
##########
@@ -0,0 +1,173 @@
+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.GatewayServiceDefaultConfig.*;
+
+public class GatewayServiceChannelConfig {
+ public enum ChannelType {
+ GRPC_SERVER, POLL_GRPC, SHARED_FILE
Review Comment:
use the new formatter of Helix and make it one per line.
##########
helix-gateway/src/main/java/org/apache/helix/gateway/channel/HelixGatewayServiceChannelFactory.java:
##########
@@ -0,0 +1,42 @@
+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,
Review Comment:
Do we have combination of it as input / output?
##########
helix-gateway/src/main/java/org/apache/helix/gateway/channel/GatewayServiceChannelConfig.java:
##########
@@ -0,0 +1,173 @@
+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.GatewayServiceDefaultConfig.*;
+
+public class GatewayServiceChannelConfig {
+ public enum ChannelType {
+ GRPC_SERVER, POLL_GRPC, SHARED_FILE
+ }
+
+ // service configs
+ // channel type for participant liveness detection
+ private ChannelType _participantConnectionChannelType;
+ // channel for sending and receiving shard state transition request and shard state response
+ private ChannelType _shardStatenChannelType;
+
+ // grpc server configs
+ private final int _grpcServerPort;
+ private final int _serverHeartBeatInterval;
+ private final int _maxAllowedClientHeartBeatInterval;
+ private final int _clientTimeout;
+ private final boolean _enableReflectionService;
+
+ // poll mode config
+ private final int _pollIntervalSec;
+ // TODO: configs for pull mode grpc client
+
+ // TODO: configs for pull mode with file
+
+ // getters
+ public ChannelType getParticipantConnectionChannelType() {
+ return _participantConnectionChannelType;
+ }
+
+ public ChannelType getShardStatenChannelType() {
+ return _shardStatenChannelType;
+ }
+
+ public int getGrpcServerPort() {
+ return _grpcServerPort;
+ }
+
+ public int getServerHeartBeatInterval() {
+ return _serverHeartBeatInterval;
+ }
+
+ public int getMaxAllowedClientHeartBeatInterval() {
+ return _maxAllowedClientHeartBeatInterval;
+ }
+
+ public int getClientTimeout() {
+ return _clientTimeout;
+ }
+
+ public boolean getEnableReflectionService() {
+ return _enableReflectionService;
+ }
+
+ public int getPollIntervalSec() {
+ return _pollIntervalSec;
+ }
+
+ public GatewayServiceChannelConfig(int grpcServerPort, ChannelType participantConnectionChannelType,
Review Comment:
If you are using builder, why we make it public?
##########
helix-gateway/src/main/java/org/apache/helix/gateway/channel/GatewayServiceChannelConfig.java:
##########
@@ -0,0 +1,173 @@
+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.GatewayServiceDefaultConfig.*;
+
+public class GatewayServiceChannelConfig {
+ public enum ChannelType {
+ GRPC_SERVER, POLL_GRPC, SHARED_FILE
Review Comment:
Also let's define right axis of the these types:
grpc as long lived (push),
grpc as short lived (poll),
file based (poll).
##########
helix-gateway/src/main/java/org/apache/helix/gateway/service/GatewayServiceManager.java:
##########
@@ -127,8 +132,13 @@ public void run() {
}
}
- public HelixGatewayServiceChannel getHelixGatewayServiceProcessor() {
- return _gatewayServiceChannel;
+
+ public void startService() throws IOException {
+ _gatewayServiceChannel.start();
+ }
+
+ public void stopService() {
+ _gatewayServiceChannel.stop();
Review Comment:
You just stop the channel. processor needs to be stopped as well. And clean the map.
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #2883:
URL: https://github.com/apache/helix/pull/2883
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2883:
URL: https://github.com/apache/helix/pull/2883#discussion_r1725947492
##########
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:
TFTR. Updated
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2883:
URL: https://github.com/apache/helix/pull/2883#discussion_r1719366767
##########
helix-gateway/src/main/java/org/apache/helix/gateway/channel/GatewayServiceChannelConfig.java:
##########
@@ -0,0 +1,173 @@
+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.GatewayServiceDefaultConfig.*;
+
+public class GatewayServiceChannelConfig {
+ public enum ChannelType {
+ GRPC_SERVER, POLL_GRPC, SHARED_FILE
Review Comment:
Long lived grpc stream may not necessary be push mode. We could maintain a stream connection with some endpoint to poll heath state and shard state.
Similarly, push mode can be unary grpc as well.
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2883:
URL: https://github.com/apache/helix/pull/2883#discussion_r1719363679
##########
helix-gateway/src/main/java/org/apache/helix/gateway/HelixGatewayMain.java:
##########
@@ -38,16 +37,11 @@ private HelixGatewayMain() {
public static void main(String[] args) throws InterruptedException, IOException {
// Create a new server to listen on port 50051
- GatewayServiceManager manager = new GatewayServiceManager();
- Server server = new HelixGatewayGrpcServerBuilder().setPort(50051)
- .setGrpcService((HelixGatewayServiceGrpcService)manager.getHelixGatewayServiceProcessor())
- .build();
-
- server.start();
- System.out.println("Server started, listening on " + server.getPort());
+ GatewayServiceChannelConfig.GatewayServiceProcessorConfigBuilder builder = new GatewayServiceChannelConfig.GatewayServiceProcessorConfigBuilder();
+ GatewayServiceManager manager = new GatewayServiceManager(args[0],
+ builder.setGrpcServerPort(50051).build());
Review Comment:
TFTR. Updated
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #2883:
URL: https://github.com/apache/helix/pull/2883#discussion_r1727576301
##########
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:
This else clause is also not necessary.
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Create Gateway service channel factory [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #2883:
URL: https://github.com/apache/helix/pull/2883#discussion_r1727604454
##########
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:
if it is PUSH_MODE but not GRPC_SERVER, we dont want to initiate anything. I feel like we still want the else
--
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-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org