You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2021/06/14 08:16:49 UTC
[GitHub] [helix] huizhilu opened a new pull request #1796: Add message util to create messages
huizhilu opened a new pull request #1796:
URL: https://github.com/apache/helix/pull/1796
### Issues
- [ ] My PR addresses the following Helix issues and references them in the PR description:
Resolves #1795
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
Message creation methods are private in message generation phase. Management mode stage will also need message generation methods to create ST cancellation and participant status change messages.
Message util will help with the purposes.
This PR moves the common message creation logic to a message util so multiple stages can reuse the code.
### 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.
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
[GitHub] [helix] huizhilu merged pull request #1796: Add message util to create messages
Posted by GitBox <gi...@apache.org>.
huizhilu merged pull request #1796:
URL: https://github.com/apache/helix/pull/1796
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1796: Add message util to create messages
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1796:
URL: https://github.com/apache/helix/pull/1796#discussion_r651343080
##########
File path: helix-core/src/main/java/org/apache/helix/util/MessageUtil.java
##########
@@ -0,0 +1,135 @@
+package org.apache.helix.util;
+
+/*
+ * 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 java.util.UUID;
+
+import org.apache.helix.HelixManager;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Resource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Message utils to operate on message such creating messages.
+ */
+public class MessageUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(MessageUtil.class);
+
+ // TODO: Make the message retry count configurable through the Cluster Config or IdealStates.
+ public final static int DEFAULT_STATE_TRANSITION_MESSAGE_RETRY_COUNT = 3;
+
+ public static Message createStateTransitionCancellationMessage(HelixManager manager,
+ Resource resource, String partitionName, String instanceName, String sessionId,
+ String stateModelDefName, String fromState, String toState, String nextState,
+ Message cancellationMessage, boolean isCancellationEnabled, String currentState) {
+ if (isCancellationEnabled && cancellationMessage == null) {
+ LOG.info("Create cancellation message of the state transition for {}.{} on {}, "
+ + "currentState: {}, nextState: {}, toState: {}", resource.getResourceName(),
+ partitionName, instanceName, currentState, nextState == null ? "N/A" : nextState,
+ toState);
+
+ Message message =
+ createStateTransitionMessage(Message.MessageType.STATE_TRANSITION_CANCELLATION, manager,
+ resource, partitionName, instanceName, currentState, nextState, sessionId,
+ stateModelDefName);
+
+ message.setFromState(fromState);
+ message.setToState(toState);
+ return message;
+ }
+
+ return null;
+ }
+
+ public static Message createStateTransitionMessage(HelixManager manager, Resource resource,
+ String partitionName, String instanceName, String currentState, String nextState,
+ String sessionId, String stateModelDefName) {
+ Message message =
+ createStateTransitionMessage(Message.MessageType.STATE_TRANSITION, manager, resource,
+ partitionName, instanceName, currentState, nextState, sessionId, stateModelDefName);
+
+ // Set the retry count for state transition messages.
+ // TODO: make the retry count configurable in ClusterConfig or IdealState
+ message.setRetryCount(DEFAULT_STATE_TRANSITION_MESSAGE_RETRY_COUNT);
+
+ if (resource.getResourceGroupName() != null) {
+ message.setResourceGroupName(resource.getResourceGroupName());
+ }
+ if (resource.getResourceTag() != null) {
+ message.setResourceTag(resource.getResourceTag());
+ }
+
+ return message;
+ }
+
+ /**
+ * Creates a message to change participant status
+ * {@link org.apache.helix.model.LiveInstance.LiveInstanceStatus}
+ *
+ * @param currentState current status of the live instance
+ * @param nextState next status that will be changed to
+ * @param manager Helix manager
+ * @param instanceName target instance name
+ * @param sessionId target instance session id
+ * @return participant status change message
+ */
+ public static Message createStatusChangeMessage(LiveInstance.LiveInstanceStatus currentState,
+ LiveInstance.LiveInstanceStatus nextState, HelixManager manager, String instanceName,
+ String sessionId) {
+ return createBasicMessage(Message.MessageType.PARTICIPANT_STATUS_CHANGE, manager, instanceName,
+ currentState.toString(), nextState.name(), sessionId);
Review comment:
currentState**.toString()**, nextState**.name()**
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1796: Add message util to create messages
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1796:
URL: https://github.com/apache/helix/pull/1796#discussion_r651343080
##########
File path: helix-core/src/main/java/org/apache/helix/util/MessageUtil.java
##########
@@ -0,0 +1,135 @@
+package org.apache.helix.util;
+
+/*
+ * 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 java.util.UUID;
+
+import org.apache.helix.HelixManager;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Resource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Message utils to operate on message such creating messages.
+ */
+public class MessageUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(MessageUtil.class);
+
+ // TODO: Make the message retry count configurable through the Cluster Config or IdealStates.
+ public final static int DEFAULT_STATE_TRANSITION_MESSAGE_RETRY_COUNT = 3;
+
+ public static Message createStateTransitionCancellationMessage(HelixManager manager,
+ Resource resource, String partitionName, String instanceName, String sessionId,
+ String stateModelDefName, String fromState, String toState, String nextState,
+ Message cancellationMessage, boolean isCancellationEnabled, String currentState) {
+ if (isCancellationEnabled && cancellationMessage == null) {
+ LOG.info("Create cancellation message of the state transition for {}.{} on {}, "
+ + "currentState: {}, nextState: {}, toState: {}", resource.getResourceName(),
+ partitionName, instanceName, currentState, nextState == null ? "N/A" : nextState,
+ toState);
+
+ Message message =
+ createStateTransitionMessage(Message.MessageType.STATE_TRANSITION_CANCELLATION, manager,
+ resource, partitionName, instanceName, currentState, nextState, sessionId,
+ stateModelDefName);
+
+ message.setFromState(fromState);
+ message.setToState(toState);
+ return message;
+ }
+
+ return null;
+ }
+
+ public static Message createStateTransitionMessage(HelixManager manager, Resource resource,
+ String partitionName, String instanceName, String currentState, String nextState,
+ String sessionId, String stateModelDefName) {
+ Message message =
+ createStateTransitionMessage(Message.MessageType.STATE_TRANSITION, manager, resource,
+ partitionName, instanceName, currentState, nextState, sessionId, stateModelDefName);
+
+ // Set the retry count for state transition messages.
+ // TODO: make the retry count configurable in ClusterConfig or IdealState
+ message.setRetryCount(DEFAULT_STATE_TRANSITION_MESSAGE_RETRY_COUNT);
+
+ if (resource.getResourceGroupName() != null) {
+ message.setResourceGroupName(resource.getResourceGroupName());
+ }
+ if (resource.getResourceTag() != null) {
+ message.setResourceTag(resource.getResourceTag());
+ }
+
+ return message;
+ }
+
+ /**
+ * Creates a message to change participant status
+ * {@link org.apache.helix.model.LiveInstance.LiveInstanceStatus}
+ *
+ * @param currentState current status of the live instance
+ * @param nextState next status that will be changed to
+ * @param manager Helix manager
+ * @param instanceName target instance name
+ * @param sessionId target instance session id
+ * @return participant status change message
+ */
+ public static Message createStatusChangeMessage(LiveInstance.LiveInstanceStatus currentState,
+ LiveInstance.LiveInstanceStatus nextState, HelixManager manager, String instanceName,
+ String sessionId) {
+ return createBasicMessage(Message.MessageType.PARTICIPANT_STATUS_CHANGE, manager, instanceName,
+ currentState.toString(), nextState.name(), sessionId);
Review comment:
currentState.**toString**(), nextState.**name**()
Let's use name() for all of them.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] huizhilu commented on pull request #1796: Add message util to create messages
Posted by GitBox <gi...@apache.org>.
huizhilu commented on pull request #1796:
URL: https://github.com/apache/helix/pull/1796#issuecomment-861158356
This PR is ready to be merged, approved by @alirezazamani
Message creation methods are private in message generation phase. Management mode stage will also need message generation methods to create ST cancellation and participant status change messages.
Message util will help with the purposes.
This commit moves the common message creation logic to a message util so multiple stages can reuse the code.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1796: Add message util to create messages
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1796:
URL: https://github.com/apache/helix/pull/1796#discussion_r651341118
##########
File path: helix-core/src/main/java/org/apache/helix/util/MessageUtil.java
##########
@@ -0,0 +1,134 @@
+package org.apache.helix.util;
+
+/*
+ * 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 java.util.UUID;
+
+import org.apache.helix.HelixManager;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Resource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Message utils to operate on message such creating messages.
+ */
+public class MessageUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(MessageUtil.class);
+
+ // TODO: Make the message retry count configurable through the Cluster Config or IdealStates.
+ public final static int DEFAULT_STATE_TRANSITION_MESSAGE_RETRY_COUNT = 3;
+
+ public static Message createStateTransitionCancellationMessage(HelixManager manager,
+ Resource resource, String partitionName, String instanceName, String sessionId,
+ String stateModelDefName, String fromState, String toState, String nextState,
+ Message cancellationMessage, boolean isCancellationEnabled, String currentState) {
+ if (isCancellationEnabled && cancellationMessage == null) {
+ LOG.info("Create cancellation message of the state transition for {}.{} on {}, "
+ + "currentState: {}, nextState: {}, toState: {}", resource.getResourceName(),
+ partitionName, instanceName, currentState, nextState == null ? "N/A" : nextState,
+ toState);
+
+ Message message =
+ createStateTransitionMessage(Message.MessageType.STATE_TRANSITION_CANCELLATION, manager, resource,
+ partitionName, instanceName, currentState, nextState, sessionId, stateModelDefName);
+
+ message.setFromState(fromState);
+ message.setToState(toState);
+ return message;
+ }
+
+ return null;
+ }
+
+ public static Message createStateTransitionMessage(HelixManager manager, Resource resource,
+ String partitionName, String instanceName, String currentState, String nextState,
+ String sessionId, String stateModelDefName) {
+ Message message =
+ createStateTransitionMessage(Message.MessageType.STATE_TRANSITION, manager, resource, partitionName,
+ instanceName, currentState, nextState, sessionId, stateModelDefName);
+
+ // Set the retry count for state transition messages.
+ // TODO: make the retry count configurable in ClusterConfig or IdealState
+ message.setRetryCount(DEFAULT_STATE_TRANSITION_MESSAGE_RETRY_COUNT);
+
+ if (resource.getResourceGroupName() != null) {
+ message.setResourceGroupName(resource.getResourceGroupName());
+ }
+ if (resource.getResourceTag() != null) {
+ message.setResourceTag(resource.getResourceTag());
+ }
+
+ return message;
+ }
+
+ /**
+ * Creates a message to change participant status
+ * {@link org.apache.helix.model.LiveInstance.LiveInstanceStatus}
+ *
+ * @param currentState current status of the live instance
+ * @param nextState next status that will be changed to
+ * @param manager Helix manager
+ * @param instanceName target instance name
+ * @param sessionId target instance session id
+ * @return participant status change message
+ */
+ public static Message createStatusChangeMessage(LiveInstance.LiveInstanceStatus currentState,
+ LiveInstance.LiveInstanceStatus nextState, HelixManager manager, String instanceName,
+ String sessionId) {
+ return createBasicMessage(Message.MessageType.PARTICIPANT_STATUS_CHANGE, manager,
+ instanceName, currentState.toString(), nextState.name(), sessionId);
+ }
+
+ /* Creates a message that that has the least required fields. */
+ private static Message createBasicMessage(Message.MessageType messageType, HelixManager manager,
Review comment:
That is fine, 2 parameters are still better than one HelixManager. The HelixManager is usually a very critical object. We should avoid sending it everywhere.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] huizhilu commented on a change in pull request #1796: Add message util to create messages
Posted by GitBox <gi...@apache.org>.
huizhilu commented on a change in pull request #1796:
URL: https://github.com/apache/helix/pull/1796#discussion_r651334672
##########
File path: helix-core/src/main/java/org/apache/helix/util/MessageUtil.java
##########
@@ -0,0 +1,134 @@
+package org.apache.helix.util;
+
+/*
+ * 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 java.util.UUID;
+
+import org.apache.helix.HelixManager;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Resource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Message utils to operate on message such creating messages.
+ */
+public class MessageUtil {
Review comment:
I thought about it. The advantage of using builder pattern is it supports different combinations of the arguments.
However, for the message creation methods, a fixed number of arguments/fields are required, eg:
```
Resource resource, String partitionName, String instanceName, String sessionId,
String stateModelDefName, String fromState, String toState, String nextState
```
Using the method arguments gives a reminder that we need to pass in the arguments, which builder pattern does not. I can also imagine that it'll need more code to validate the arguments in the builder. Using builder pattern does not give better controller of construction for this case here. And it would also have a long list of setMethod():
```
newBuilder()
.setA()
.setB()
.setC()
...
.setX()
```
So I chose to keep the original long list of arguments.
##########
File path: helix-core/src/main/java/org/apache/helix/util/MessageUtil.java
##########
@@ -0,0 +1,134 @@
+package org.apache.helix.util;
+
+/*
+ * 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 java.util.UUID;
+
+import org.apache.helix.HelixManager;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Resource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Message utils to operate on message such creating messages.
+ */
+public class MessageUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(MessageUtil.class);
+
+ // TODO: Make the message retry count configurable through the Cluster Config or IdealStates.
+ public final static int DEFAULT_STATE_TRANSITION_MESSAGE_RETRY_COUNT = 3;
+
+ public static Message createStateTransitionCancellationMessage(HelixManager manager,
+ Resource resource, String partitionName, String instanceName, String sessionId,
+ String stateModelDefName, String fromState, String toState, String nextState,
+ Message cancellationMessage, boolean isCancellationEnabled, String currentState) {
+ if (isCancellationEnabled && cancellationMessage == null) {
+ LOG.info("Create cancellation message of the state transition for {}.{} on {}, "
+ + "currentState: {}, nextState: {}, toState: {}", resource.getResourceName(),
+ partitionName, instanceName, currentState, nextState == null ? "N/A" : nextState,
+ toState);
+
+ Message message =
+ createStateTransitionMessage(Message.MessageType.STATE_TRANSITION_CANCELLATION, manager, resource,
+ partitionName, instanceName, currentState, nextState, sessionId, stateModelDefName);
+
+ message.setFromState(fromState);
+ message.setToState(toState);
+ return message;
+ }
+
+ return null;
+ }
+
+ public static Message createStateTransitionMessage(HelixManager manager, Resource resource,
+ String partitionName, String instanceName, String currentState, String nextState,
+ String sessionId, String stateModelDefName) {
+ Message message =
+ createStateTransitionMessage(Message.MessageType.STATE_TRANSITION, manager, resource, partitionName,
+ instanceName, currentState, nextState, sessionId, stateModelDefName);
+
+ // Set the retry count for state transition messages.
+ // TODO: make the retry count configurable in ClusterConfig or IdealState
+ message.setRetryCount(DEFAULT_STATE_TRANSITION_MESSAGE_RETRY_COUNT);
+
+ if (resource.getResourceGroupName() != null) {
+ message.setResourceGroupName(resource.getResourceGroupName());
+ }
+ if (resource.getResourceTag() != null) {
+ message.setResourceTag(resource.getResourceTag());
+ }
+
+ return message;
+ }
+
+ /**
+ * Creates a message to change participant status
+ * {@link org.apache.helix.model.LiveInstance.LiveInstanceStatus}
+ *
+ * @param currentState current status of the live instance
+ * @param nextState next status that will be changed to
+ * @param manager Helix manager
+ * @param instanceName target instance name
+ * @param sessionId target instance session id
+ * @return participant status change message
+ */
+ public static Message createStatusChangeMessage(LiveInstance.LiveInstanceStatus currentState,
+ LiveInstance.LiveInstanceStatus nextState, HelixManager manager, String instanceName,
+ String sessionId) {
+ return createBasicMessage(Message.MessageType.PARTICIPANT_STATUS_CHANGE, manager,
+ instanceName, currentState.toString(), nextState.name(), sessionId);
+ }
+
+ /* Creates a message that that has the least required fields. */
+ private static Message createBasicMessage(Message.MessageType messageType, HelixManager manager,
Review comment:
It also provides source instance name. `message.setSrcName(manager.getInstanceName());`
We can convert the manager param -> 2 params: session and instanceName. I was thinking it adds up the number of params.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1796: Add message util to create messages
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1796:
URL: https://github.com/apache/helix/pull/1796#discussion_r651342521
##########
File path: helix-core/src/main/java/org/apache/helix/util/MessageUtil.java
##########
@@ -0,0 +1,134 @@
+package org.apache.helix.util;
+
+/*
+ * 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 java.util.UUID;
+
+import org.apache.helix.HelixManager;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Resource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Message utils to operate on message such creating messages.
+ */
+public class MessageUtil {
Review comment:
The builder also helps to avoid too many constructors if we keep adding parameters and there are different combinations. But I don't think it is a major concern here. Let's keep the current implementation.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] jiajunwang commented on a change in pull request #1796: Add message util to create messages
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1796:
URL: https://github.com/apache/helix/pull/1796#discussion_r651278598
##########
File path: helix-core/src/main/java/org/apache/helix/util/MessageUtil.java
##########
@@ -0,0 +1,134 @@
+package org.apache.helix.util;
+
+/*
+ * 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 java.util.UUID;
+
+import org.apache.helix.HelixManager;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Resource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Message utils to operate on message such creating messages.
+ */
+public class MessageUtil {
+ private static final Logger LOG = LoggerFactory.getLogger(MessageUtil.class);
+
+ // TODO: Make the message retry count configurable through the Cluster Config or IdealStates.
+ public final static int DEFAULT_STATE_TRANSITION_MESSAGE_RETRY_COUNT = 3;
+
+ public static Message createStateTransitionCancellationMessage(HelixManager manager,
+ Resource resource, String partitionName, String instanceName, String sessionId,
+ String stateModelDefName, String fromState, String toState, String nextState,
+ Message cancellationMessage, boolean isCancellationEnabled, String currentState) {
+ if (isCancellationEnabled && cancellationMessage == null) {
+ LOG.info("Create cancellation message of the state transition for {}.{} on {}, "
+ + "currentState: {}, nextState: {}, toState: {}", resource.getResourceName(),
+ partitionName, instanceName, currentState, nextState == null ? "N/A" : nextState,
+ toState);
+
+ Message message =
+ createStateTransitionMessage(Message.MessageType.STATE_TRANSITION_CANCELLATION, manager, resource,
+ partitionName, instanceName, currentState, nextState, sessionId, stateModelDefName);
+
+ message.setFromState(fromState);
+ message.setToState(toState);
+ return message;
+ }
+
+ return null;
+ }
+
+ public static Message createStateTransitionMessage(HelixManager manager, Resource resource,
+ String partitionName, String instanceName, String currentState, String nextState,
+ String sessionId, String stateModelDefName) {
+ Message message =
+ createStateTransitionMessage(Message.MessageType.STATE_TRANSITION, manager, resource, partitionName,
+ instanceName, currentState, nextState, sessionId, stateModelDefName);
+
+ // Set the retry count for state transition messages.
+ // TODO: make the retry count configurable in ClusterConfig or IdealState
+ message.setRetryCount(DEFAULT_STATE_TRANSITION_MESSAGE_RETRY_COUNT);
+
+ if (resource.getResourceGroupName() != null) {
+ message.setResourceGroupName(resource.getResourceGroupName());
+ }
+ if (resource.getResourceTag() != null) {
+ message.setResourceTag(resource.getResourceTag());
+ }
+
+ return message;
+ }
+
+ /**
+ * Creates a message to change participant status
+ * {@link org.apache.helix.model.LiveInstance.LiveInstanceStatus}
+ *
+ * @param currentState current status of the live instance
+ * @param nextState next status that will be changed to
+ * @param manager Helix manager
+ * @param instanceName target instance name
+ * @param sessionId target instance session id
+ * @return participant status change message
+ */
+ public static Message createStatusChangeMessage(LiveInstance.LiveInstanceStatus currentState,
+ LiveInstance.LiveInstanceStatus nextState, HelixManager manager, String instanceName,
+ String sessionId) {
+ return createBasicMessage(Message.MessageType.PARTICIPANT_STATUS_CHANGE, manager,
+ instanceName, currentState.toString(), nextState.name(), sessionId);
+ }
+
+ /* Creates a message that that has the least required fields. */
+ private static Message createBasicMessage(Message.MessageType messageType, HelixManager manager,
Review comment:
It seems that this is the root constructor. Since HelixManager is only to provide session ID, shall we just change the input to be helixManangerSessionId?
##########
File path: helix-core/src/main/java/org/apache/helix/util/MessageUtil.java
##########
@@ -0,0 +1,134 @@
+package org.apache.helix.util;
+
+/*
+ * 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 java.util.UUID;
+
+import org.apache.helix.HelixManager;
+import org.apache.helix.model.LiveInstance;
+import org.apache.helix.model.Message;
+import org.apache.helix.model.Resource;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Message utils to operate on message such creating messages.
+ */
+public class MessageUtil {
Review comment:
I think it would be a more elegant design if the logic in this util is done as MessageBuilder. Mainly to avoid long parameter lists and easier for future extension.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] junkaixue commented on a change in pull request #1796: Add message util to create messages
Posted by GitBox <gi...@apache.org>.
junkaixue commented on a change in pull request #1796:
URL: https://github.com/apache/helix/pull/1796#discussion_r651157980
##########
File path: helix-core/src/main/java/org/apache/helix/model/Message.java
##########
@@ -82,6 +82,7 @@
RESOURCE_TAG,
FROM_STATE,
TO_STATE,
+ TO_STATUS,
Review comment:
Add a field could be make the message logic more complicated. You can let the from state as "NORMAL" to "FREEZE" or something.
I think you are trying to make an extra field to differentiate whether it is freeze message or not. But I would suggest to use our message type to achieve that goal.
--
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org