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 2020/08/10 18:43:50 UTC
[GitHub] [helix] jiajunwang commented on a change in pull request #1066: Prevent parallel controller pipelines run causing two master replicas
jiajunwang commented on a change in pull request #1066:
URL: https://github.com/apache/helix/pull/1066#discussion_r468104455
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/datamodel/SessionAwareZNRecord.java
##########
@@ -0,0 +1,68 @@
+package org.apache.helix.zookeeper.datamodel;
+
+/*
+ * 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.codehaus.jackson.annotate.JsonIgnore;
+
+/**
+ * A class represents a session aware ZNRecord: the ZNRecord should be written to zk by
+ * the expected zk session. When the ZNRecord is being written to zk, if the actual
+ * zk session id doesn't match the expected zk session id set in the {@code SessionAwareZNRecord},
+ * writing to zk will fail. It is supposed to be used within Helix only.
+ * <p>
+ * If this ZNRecord is not supposed to be written only by the expected zk session,
+ * {@link ZNRecord} is recommended to use.
+ */
+public class SessionAwareZNRecord extends ZNRecord {
+ @JsonIgnore
+ private String expectedSessionId;
+
+ public SessionAwareZNRecord(String id) {
+ super(id);
+ }
+
+ public SessionAwareZNRecord(ZNRecord record) {
+ super(record);
+ }
+
+ public SessionAwareZNRecord(ZNRecord record, String id) {
Review comment:
Is this one needed?
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/datamodel/SessionAwareZNRecord.java
##########
@@ -0,0 +1,68 @@
+package org.apache.helix.zookeeper.datamodel;
+
+/*
+ * 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.codehaus.jackson.annotate.JsonIgnore;
+
+/**
+ * A class represents a session aware ZNRecord: the ZNRecord should be written to zk by
+ * the expected zk session. When the ZNRecord is being written to zk, if the actual
+ * zk session id doesn't match the expected zk session id set in the {@code SessionAwareZNRecord},
+ * writing to zk will fail. It is supposed to be used within Helix only.
+ * <p>
+ * If this ZNRecord is not supposed to be written only by the expected zk session,
+ * {@link ZNRecord} is recommended to use.
+ */
+public class SessionAwareZNRecord extends ZNRecord {
+ @JsonIgnore
+ private String expectedSessionId;
+
+ public SessionAwareZNRecord(String id) {
Review comment:
Is this one needed?
##########
File path: helix-core/src/main/java/org/apache/helix/HelixProperty.java
##########
@@ -148,7 +154,12 @@ public String toString() {
}
}
- private Stat _stat;
+ protected Stat _stat;
+
+ public HelixProperty() {
+ _record = DEFAULT_ZNRECORD;
+ _stat = DEFAULT_STAT;
+ }
Review comment:
How about do this?
public HelixProperty(SessionAwareZNRecord record, String id) {
_record = record;
_stat = new Stat(_record.getVersion(), _record.getCreationTime(), _record.getModifiedTime(),
_record.getEphemeralOwner());
}
##########
File path: helix-core/src/main/java/org/apache/helix/controller/stages/AttributeName.java
##########
@@ -40,5 +40,8 @@
PipelineType,
LastRebalanceFinishTimeStamp,
ControllerDataProvider,
- STATEFUL_REBALANCER
+ STATEFUL_REBALANCER,
+
+ /** Event should be processed by the expected session. */
Review comment:
This claim is not related to this field but with the internal logic. Let's just say this is the session that in which we get the event.
##########
File path: helix-core/src/main/java/org/apache/helix/controller/GenericHelixController.java
##########
@@ -695,12 +697,6 @@ private void handleEvent(ClusterEvent event, BaseControllerDataProvider dataProv
event.addAttribute(AttributeName.STATEFUL_REBALANCER.name(),
_rebalancerRef.getRebalancer(manager));
- if (!manager.isLeader()) {
Review comment:
Delaying this check might cause confusing and seems not necessary to this PR. Let's don't change it in this PR even if desired.
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -419,6 +420,17 @@ void addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
Long getSessionStartTime();
+ /**
+ * Checks whether the cluster manager is leader and returns the session ID associated to the
+ * connection of cluster data store, if and only if it is leader.
+ *
+ * @return {@code Optional<String>} session ID is present inside the {@code Optional} object
+ * if the cluster manager is leader. Otherwise, returns an empty {@code Optional} object.
+ */
+ default Optional<String> getSessionIdIfLead() {
Review comment:
Let me open another thread for this change. I would like to hear your idea.
High-levelly, the Session Id is tightly bounded with ZK. But what if we have other HelixManager that are not based on ZK? So I am thinking that maybe we should make the session a more generic concept. For the management connection that has a session concept, we implement it and return real session. For the other management connection that does not care about session, we just return a fake constant session so no one needs to try catch on this method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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