[GitHub] helix pull request #266: Propose design for aggregated cluster view service
Github user kishoreg commented on a diff in the pull request: https://github.com/apache/helix/pull/266#discussion_r226866903 --- Diff: designs/aggregated-cluster-view/design.md --- @@ -0,0 +1,353 @@ +Aggregated Cluster View Design +== + +## Introduction +Currently Helix organize information by cluster - clusters are autonomous entities that holds resource / node information. +In real practice, a helix client might need to access aggregated information of helix clusters from different data center regions for management or coordination purpose. --- End diff -- examples of real use cases? ---
[GitHub] helix issue #248: helix manager should support getting metadata store connec...
Github user kishoreg commented on the issue: https://github.com/apache/helix/pull/248 what is the purpose of this api and what will the caller do with the return value? ---
[GitHub] helix pull request #169: Two Performance Improvement for Rebalance pipeline
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/169#discussion_r177523538
--- Diff:
helix-core/src/main/java/org/apache/helix/common/caches/BasicClusterDataCache.java
---
@@ -104,6 +108,52 @@ public void refresh(HelixDataAccessor accessor) {
}
}
+ /**
+ * Selective update Helix Cache by version
+ * @param accessor the HelixDataAccessor
+ * @param reloadKeys keys needs to be reload
+ * @param cachedKeys keys already exists in the cache
+ * @param cachedPropertyMap cached map of propertykey -> property object
+ * @param the type of metadata
+ * @return
+ */
+ public static Map<PropertyKey, T>
updateReloadProperties(
+ HelixDataAccessor accessor, List reloadKeys,
List cachedKeys,
--- End diff --
how will cachedKeys be different from cachedPropertyMap.keySet?
---
[GitHub] helix pull request #169: Two Performance Improvement for Rebalance pipeline
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/169#discussion_r177523236
--- Diff:
helix-core/src/main/java/org/apache/helix/common/caches/BasicClusterDataCache.java
---
@@ -104,6 +108,52 @@ public void refresh(HelixDataAccessor accessor) {
}
}
+ /**
+ * Selective update Helix Cache by version
+ * @param accessor the HelixDataAccessor
+ * @param reloadKeys keys needs to be reload
+ * @param cachedKeys keys already exists in the cache
+ * @param cachedPropertyMap cached map of propertykey -> property object
+ * @param the type of metadata
+ * @return
+ */
+ public static Map<PropertyKey, T>
updateReloadProperties(
+ HelixDataAccessor accessor, List reloadKeys,
List cachedKeys,
+ Map<PropertyKey, T> cachedPropertyMap) {
+// All new entries from zk not cached locally yet should be read from
ZK.
+Map<PropertyKey, T> refreshedPropertyMap = Maps.newHashMap();
+List stats = accessor.getPropertyStats(cachedKeys);
+for (int i = 0; i < cachedKeys.size(); i++) {
+ PropertyKey key = cachedKeys.get(i);
+ HelixProperty.Stat stat = stats.get(i);
+ if (stat != null) {
+T property = cachedPropertyMap.get(key);
+if (property != null && property.getBucketSize() == 0 &&
property.getStat().equals(stat)) {
+ refreshedPropertyMap.put(key, property);
+} else {
+ // need update from zk
+ reloadKeys.add(key);
+}
+ } else {
+LOG.warn("stat is null for key: " + key);
+reloadKeys.add(key);
+ }
+}
+
+List reloadedProperty = accessor.getProperty(reloadKeys, true);
+Iterator csKeyIter = reloadKeys.iterator();
+for (T property : reloadedProperty) {
+ PropertyKey key = csKeyIter.next();
+ if (property != null) {
+refreshedPropertyMap.put(key, property);
+ } else {
+LOG.warn("CurrentState null for key: " + key);
--- End diff --
this is not related to currentstate right
---
[GitHub] helix pull request #169: Two Performance Improvement for Rebalance pipeline
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/169#discussion_r177522574
--- Diff:
helix-core/src/main/java/org/apache/helix/common/caches/BasicClusterDataCache.java
---
@@ -104,6 +108,52 @@ public void refresh(HelixDataAccessor accessor) {
}
}
+ /**
+ * Selective update Helix Cache by version
+ * @param accessor the HelixDataAccessor
+ * @param reloadKeys keys needs to be reload
+ * @param cachedKeys keys already exists in the cache
+ * @param cachedPropertyMap cached map of propertykey -> property object
+ * @param the type of metadata
+ * @return
+ */
+ public static Map<PropertyKey, T>
updateReloadProperties(
--- End diff --
If this is a public static method, why is this in this class?
---
[GitHub] helix pull request #169: Two Performance Improvement for Rebalance pipeline
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/169#discussion_r177523148
--- Diff:
helix-core/src/main/java/org/apache/helix/common/caches/BasicClusterDataCache.java
---
@@ -104,6 +108,52 @@ public void refresh(HelixDataAccessor accessor) {
}
}
+ /**
+ * Selective update Helix Cache by version
+ * @param accessor the HelixDataAccessor
+ * @param reloadKeys keys needs to be reload
+ * @param cachedKeys keys already exists in the cache
+ * @param cachedPropertyMap cached map of propertykey -> property object
+ * @param the type of metadata
+ * @return
+ */
+ public static Map<PropertyKey, T>
updateReloadProperties(
+ HelixDataAccessor accessor, List reloadKeys,
List cachedKeys,
+ Map<PropertyKey, T> cachedPropertyMap) {
+// All new entries from zk not cached locally yet should be read from
ZK.
+Map<PropertyKey, T> refreshedPropertyMap = Maps.newHashMap();
+List stats = accessor.getPropertyStats(cachedKeys);
+for (int i = 0; i < cachedKeys.size(); i++) {
+ PropertyKey key = cachedKeys.get(i);
+ HelixProperty.Stat stat = stats.get(i);
+ if (stat != null) {
+T property = cachedPropertyMap.get(key);
+if (property != null && property.getBucketSize() == 0 &&
property.getStat().equals(stat)) {
+ refreshedPropertyMap.put(key, property);
+} else {
+ // need update from zk
+ reloadKeys.add(key);
+}
+ } else {
+LOG.warn("stat is null for key: " + key);
+reloadKeys.add(key);
+ }
+}
+
+List reloadedProperty = accessor.getProperty(reloadKeys, true);
+Iterator csKeyIter = reloadKeys.iterator();
--- End diff --
is this method specific to current state? why are the variables named cs?
---
[GitHub] helix pull request #169: Two Performance Improvement for Rebalance pipeline
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/169#discussion_r177522908
--- Diff:
helix-core/src/main/java/org/apache/helix/common/caches/BasicClusterDataCache.java
---
@@ -104,6 +108,52 @@ public void refresh(HelixDataAccessor accessor) {
}
}
+ /**
+ * Selective update Helix Cache by version
+ * @param accessor the HelixDataAccessor
+ * @param reloadKeys keys needs to be reload
+ * @param cachedKeys keys already exists in the cache
+ * @param cachedPropertyMap cached map of propertykey -> property object
+ * @param the type of metadata
+ * @return
+ */
+ public static Map<PropertyKey, T>
updateReloadProperties(
+ HelixDataAccessor accessor, List reloadKeys,
List cachedKeys,
+ Map<PropertyKey, T> cachedPropertyMap) {
+// All new entries from zk not cached locally yet should be read from
ZK.
+Map<PropertyKey, T> refreshedPropertyMap = Maps.newHashMap();
+List stats = accessor.getPropertyStats(cachedKeys);
+for (int i = 0; i < cachedKeys.size(); i++) {
--- End diff --
what will the stat be if the znode is deleted
---
[GitHub] helix pull request #159: Improvements to Helix's Callback handling.
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/159#discussion_r177275969
--- Diff:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java ---
@@ -257,18 +308,22 @@ public void run() {
}
try {
logger.info(
-"Num callbacks merged for path:" + handler.getPath() + " :
" + mergedCallbacks);
-handler.invoke(notificationToProcess);
+"Num callbacks merged for path:" + _handler.getPath() + "
: " + mergedCallbacks);
--- End diff --
this will no longer be more than 1 right, since we dedup it when we insert
the event into the queue?
---
[GitHub] helix pull request #159: Improvements to Helix's Callback handling.
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/159#discussion_r177260701
--- Diff:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java ---
@@ -153,7 +195,12 @@ private void parseListenerProperties() {
BatchMode batchMode =
_listener.getClass().getAnnotation(BatchMode.class);
PreFetch preFetch = _listener.getClass().getAnnotation(PreFetch.class);
-String asyncBatchModeEnabled =
System.getProperty("isAsyncBatchModeEnabled");
+String asyncBatchModeEnabled =
System.getProperty("helix.callbackhandler.isAsyncBatchModeEnabled");
--- End diff --
can we make this backward compatible?
---
[GitHub] helix pull request #159: Improvements to Helix's Callback handling.
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/159#discussion_r177274948
--- Diff:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java ---
@@ -168,41 +215,41 @@ private void parseListenerProperties() {
Class listenerClass = null;
switch (_changeType) {
-case IDEAL_STATE:
- listenerClass = IdealStateChangeListener.class;
- break;
-case INSTANCE_CONFIG:
- if (_listener instanceof ConfigChangeListener) {
+ case IDEAL_STATE:
+listenerClass = IdealStateChangeListener.class;
+break;
+ case INSTANCE_CONFIG:
+if (_listener instanceof ConfigChangeListener) {
+ listenerClass = ConfigChangeListener.class;
+} else if (_listener instanceof InstanceConfigChangeListener) {
+ listenerClass = InstanceConfigChangeListener.class;
+}
+break;
+ case CLUSTER_CONFIG:
+listenerClass = ClusterConfigChangeListener.class;
+break;
+ case RESOURCE_CONFIG:
+listenerClass = ResourceConfigChangeListener.class;
+break;
+ case CONFIG:
listenerClass = ConfigChangeListener.class;
- } else if (_listener instanceof InstanceConfigChangeListener) {
-listenerClass = InstanceConfigChangeListener.class;
- }
- break;
-case CLUSTER_CONFIG:
- listenerClass = ClusterConfigChangeListener.class;
- break;
-case RESOURCE_CONFIG:
- listenerClass = ResourceConfigChangeListener.class;
- break;
-case CONFIG:
- listenerClass = ConfigChangeListener.class;
- break;
-case LIVE_INSTANCE:
- listenerClass = LiveInstanceChangeListener.class;
- break;
-case CURRENT_STATE:
- listenerClass = CurrentStateChangeListener.class;;
- break;
-case MESSAGE:
-case MESSAGES_CONTROLLER:
- listenerClass = MessageListener.class;
- break;
-case EXTERNAL_VIEW:
-case TARGET_EXTERNAL_VIEW:
- listenerClass = ExternalViewChangeListener.class;
- break;
-case CONTROLLER:
- listenerClass = ControllerChangeListener.class;
+break;
+ case LIVE_INSTANCE:
+listenerClass = LiveInstanceChangeListener.class;
+break;
+ case CURRENT_STATE:
+listenerClass = CurrentStateChangeListener.class;;
+break;
+ case MESSAGE:
+ case MESSAGES_CONTROLLER:
+listenerClass = MessageListener.class;
+break;
+ case EXTERNAL_VIEW:
+ case TARGET_EXTERNAL_VIEW:
+listenerClass = ExternalViewChangeListener.class;
+break;
+ case CONTROLLER:
+listenerClass = ControllerChangeListener.class;
}
Method callbackMethod = listenerClass.getMethods()[0];
--- End diff --
can we get the method using the explicit name.
---
[GitHub] helix pull request #159: Improvements to Helix's Callback handling.
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/159#discussion_r177257703
--- Diff:
helix-core/src/main/java/org/apache/helix/common/DedupEventBlockingQueue.java
---
@@ -0,0 +1,139 @@
+package org.apache.helix.common;
+
+/*
+ * 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 com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import java.util.Map;
+import java.util.Queue;
+import java.util.concurrent.BlockingQueue;
+
+/**
+ * A blocking queue of events, which automatically deduplicate events with
the same "type" within
+ * the queue, i.e, when putting an event into the queue, if there is
already an event with the
+ * same type existing in the queue, the new event won't be inserted into
the queue.
+ * This class is meant to be a limited implementation of the {@link
BlockingQueue} interface.
+ *
+ * T -- the Type of an event.
+ * E -- the event itself.
+ */
+public class DedupEventBlockingQueue<T, E> {
+ private final Map<T, Entry<T, E>> _eventMap;
+ private final Queue _eventQueue;
+
+ class Entry <T, E> {
+private T _type;
+private E _event;
+
+Entry (T type, E event) {
+ _type = type;
+ _event = event;
+}
+
+T getType() {
+ return _type;
+}
+
+E getEvent() {
+ return _event;
+}
+ }
+
+ /**
+ * Instantiate the queue
+ */
+ public DedupEventBlockingQueue() {
+_eventMap = Maps.newHashMap();
+_eventQueue = Lists.newLinkedList();
+ }
+
+ /**
+ * Remove all events from the queue
+ */
+ public synchronized void clear() {
+_eventMap.clear();
+_eventQueue.clear();
+ }
+
+ /**
+ * Add a single event to the queue, overwriting events with the same name
+ */
+ public synchronized void put(T type, E event) {
+Entry entry = new Entry(type, event);
+
+if (!_eventMap.containsKey(entry.getType())) {
+ // only insert to the queue if there isn't a same-typed event
already present
+ boolean result = _eventQueue.offer(entry);
+ if (!result) {
+return;
+ }
+}
+// always overwrite the existing entry in the map in case the entry is
different
+_eventMap.put((T) entry.getType(), entry);
+notify();
+ }
+
+ /**
+ * Remove an element from the front of the queue, blocking if none is
available. This method
+ * will return the most recent event seen with the oldest enqueued event
name.
+ * @return ClusterEvent at the front of the queue
+ * @throws InterruptedException if the wait for elements was interrupted
+ */
+ public synchronized E take() throws InterruptedException {
--- End diff --
synchronizing this method is not sufficient. The data structures are
getting used/modified across two methods take and get. You need to protect the
two methods using the same lock.
---
[GitHub] helix pull request #159: Improvements to Helix's Callback handling.
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/159#discussion_r177260018
--- Diff:
helix-core/src/main/java/org/apache/helix/common/DedupEventBlockingQueue.java
---
@@ -0,0 +1,139 @@
+package org.apache.helix.common;
+
+/*
+ * 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 com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import java.util.Map;
+import java.util.Queue;
+import java.util.concurrent.BlockingQueue;
+
+/**
+ * A blocking queue of events, which automatically deduplicate events with
the same "type" within
+ * the queue, i.e, when putting an event into the queue, if there is
already an event with the
+ * same type existing in the queue, the new event won't be inserted into
the queue.
+ * This class is meant to be a limited implementation of the {@link
BlockingQueue} interface.
+ *
+ * T -- the Type of an event.
+ * E -- the event itself.
+ */
+public class DedupEventBlockingQueue<T, E> {
+ private final Map<T, Entry<T, E>> _eventMap;
+ private final Queue _eventQueue;
+
+ class Entry <T, E> {
+private T _type;
+private E _event;
+
+Entry (T type, E event) {
+ _type = type;
+ _event = event;
+}
+
+T getType() {
+ return _type;
+}
+
+E getEvent() {
+ return _event;
+}
+ }
+
+ /**
+ * Instantiate the queue
+ */
+ public DedupEventBlockingQueue() {
+_eventMap = Maps.newHashMap();
+_eventQueue = Lists.newLinkedList();
+ }
+
+ /**
+ * Remove all events from the queue
+ */
+ public synchronized void clear() {
+_eventMap.clear();
+_eventQueue.clear();
+ }
+
+ /**
+ * Add a single event to the queue, overwriting events with the same name
+ */
+ public synchronized void put(T type, E event) {
+Entry entry = new Entry(type, event);
+
+if (!_eventMap.containsKey(entry.getType())) {
+ // only insert to the queue if there isn't a same-typed event
already present
+ boolean result = _eventQueue.offer(entry);
+ if (!result) {
+return;
+ }
+}
+// always overwrite the existing entry in the map in case the entry is
different
+_eventMap.put((T) entry.getType(), entry);
+notify();
+ }
+
+ /**
+ * Remove an element from the front of the queue, blocking if none is
available. This method
+ * will return the most recent event seen with the oldest enqueued event
name.
+ * @return ClusterEvent at the front of the queue
+ * @throws InterruptedException if the wait for elements was interrupted
+ */
+ public synchronized E take() throws InterruptedException {
+while (_eventQueue.isEmpty()) {
+ wait();
--- End diff --
Could we use blocking q and piggy bank on that impl?
---
[GitHub] helix pull request #155: Features and improvements to Helix RoutingTableProv...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/155#discussion_r176172589
--- Diff:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableSnapshot.java
---
@@ -0,0 +1,151 @@
+package org.apache.helix.spectator;
+
+/*
+ * 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.Collection;
+import java.util.List;
+import java.util.Set;
+import org.apache.helix.model.InstanceConfig;
+import org.apache.helix.model.LiveInstance;
+
+/**
+ * The snapshot of RoutingTable information. It is immutable, it reflects
the routing table
+ * information at the time it is generated.
+ */
+public class RoutingTableSnapshot {
--- End diff --
whats the benefit of this wrapper on RoutingTable?
---
[GitHub] helix pull request #155: Features and improvements to Helix RoutingTableProv...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/155#discussion_r176171189
--- Diff:
helix-core/src/main/java/org/apache/helix/manager/zk/CallbackHandler.java ---
@@ -576,8 +573,8 @@ public void handleDataChange(String dataPath, Object
data) {
@Override
public void handleChildChange(String parentPath, List
currentChilds) {
-logger.debug("Data change callback: child changed, path: " +
parentPath + ", current childs: "
-+ currentChilds);
+logger.info("Data change callback: child changed, path: " + parentPath
+ ", current child count: "
--- End diff --
lets avoid constructing strings to reduce GC pressure. Also, this will
pollute the logs. What are we trying to get out of these log messages
---
[GitHub] helix pull request #138: CallbackHandler to use either java config property ...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/138#discussion_r171987248
--- Diff:
helix-core/src/test/java/org/apache/helix/TestListenerCallbackBatchMode.java ---
@@ -122,21 +131,45 @@ public void testNonBatchedListener() throws Exception
{
final Listener listener = new Listener();
addListeners(listener);
+updateConfigs();
+verifyNonbatchedListeners(listener);
+removeListeners(listener);
+System.out.println("END " + methodName + " at " + new
Date(System.currentTimeMillis()));
--- End diff --
remove sysout
---
[GitHub] helix pull request #138: CallbackHandler to use either java config property ...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/138#discussion_r171987319
--- Diff:
helix-core/src/test/java/org/apache/helix/TestListenerCallbackBatchMode.java ---
@@ -122,21 +131,45 @@ public void testNonBatchedListener() throws Exception
{
final Listener listener = new Listener();
addListeners(listener);
+updateConfigs();
+verifyNonbatchedListeners(listener);
+removeListeners(listener);
+System.out.println("END " + methodName + " at " + new
Date(System.currentTimeMillis()));
+ }
+
+ @Test (dependsOnMethods = {"testNonBatchedListener",
"testBatchedListener", "testMixedListener"})
+ public void testEnableBatchedListenerByJavaProperty() throws Exception {
+String methodName = TestHelper.getTestMethodName();
+System.out.println("START " + methodName + " at " + new
Date(System.currentTimeMillis()));
--- End diff --
avoid using sysout every where
---
[GitHub] helix pull request #:
Github user kishoreg commented on the pull request: https://github.com/apache/helix/commit/bada911c7f246cf685c30323e118402cca89111d#commitcomment-26928342 In helix-core/src/main/java/org/apache/helix/GroupCommit.java: In helix-core/src/main/java/org/apache/helix/GroupCommit.java on line 142: Yes, it should be configurable and have exponential back up. But along with that, there should be a graceful way to handle network outage ---
[GitHub] helix pull request #:
Github user kishoreg commented on the pull request: https://github.com/apache/helix/commit/bada911c7f246cf685c30323e118402cca89111d#commitcomment-26928170 In helix-core/src/main/java/org/apache/helix/GroupCommit.java: In helix-core/src/main/java/org/apache/helix/GroupCommit.java on line 142: Good catch. Can you create an issue and attach the logs. At the least, we should handle this gracefully. I cant think of a straightforward solution at this point. @lei-xia any ideas? ---
[GitHub] helix pull request #87: Make map in NotificationContext synchronized
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/87#discussion_r116129454
--- Diff:
helix-core/src/main/java/org/apache/helix/messaging/handling/HelixTaskExecutor.java
---
@@ -143,6 +145,7 @@ public HelixTaskExecutor(ParticipantStatusMonitor
participantStatusMonitor) {
_hdlrFtyRegistry = new ConcurrentHashMap<String,
MsgHandlerFactoryRegistryItem>();
_executorMap = new ConcurrentHashMap<String, ExecutorService>();
_batchMessageExecutorService = Executors.newCachedThreadPool();
--- End diff --
Lets have a default value here of 40 (same as the non batch executor size)
for the thread pool size instead of setting it to cachedThreadPool.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix issue #59: helix-core: AutoRebalanceStrategy should sort by partition ...
Github user kishoreg commented on the issue: https://github.com/apache/helix/pull/59 Any update on this PR. I think we are seeing similar issues in one of our projects --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request #82: Add Test for Batch Message ThreadPool
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/82#discussion_r109814455
--- Diff:
helix-core/src/test/java/org/apache/helix/messaging/handling/TestResourceThreadpoolSize.java
---
@@ -113,6 +113,35 @@ public void TestThreadPoolSizeConfig() {
}
}
+ @Test
+ public void testBatchMessageThreadPoolSize() throws InterruptedException
{
--- End diff --
Thanks, do you see that this test case fails without my change. Might have
to run it multiple times or add a delay in batch message handler before it
executes the sub-messages to reproduce it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request #82: Add Test for Batch Message ThreadPool
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/82#discussion_r109800436
--- Diff:
helix-core/src/test/java/org/apache/helix/messaging/handling/TestResourceThreadpoolSize.java
---
@@ -113,6 +113,35 @@ public void TestThreadPoolSizeConfig() {
}
}
+ @Test
+ public void testBatchMessageThreadPoolSize() throws InterruptedException
{
--- End diff --
we need a test with number of resources with group message enabled >
threapool size. This is the scenario where current code deadlocks. This test
case does not cover that scenario
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request #81: Creating a separate threadpool to handle batchMessag...
GitHub user kishoreg opened a pull request: https://github.com/apache/helix/pull/81 Creating a separate threadpool to handle batchMessages batchMessages were using the same threadpool as sub tasks. This works as long the thread pool size is greater than the number of messages with batchMessageMode=true. If not, there is a deadlock. Consider the following scenario - - 100 messages with batchMessage=true come in, each with 10 submessages - let's say threadpool size is 40 (this is current default) - 40 message will get scheduled first. Once the execution of the thread starts, each thread now tries to execute the 10 sub-messages but in the current code we use the same threadpool to execute batch message and sub message. This means the sub-messages never get scheduled and everything gets blocked. The solution was to use a separate threadpool for state transition messages here batchMessage=true. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kishoreg/helix batch-message-bug-fix Alternatively you can review and apply these changes as the patch at: https://github.com/apache/helix/pull/81.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #81 commit 70a585aca1302aff767a91c59040ad9c94439323 Author: kishoreg <kisho...@apache.org> Date: 2017-04-03T07:10:20Z Creating a separate threadpool to handle batchMessages --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request #78: Auto compress ZNode that are greater than 1MB
GitHub user kishoreg opened a pull request: https://github.com/apache/helix/pull/78 Auto compress ZNode that are greater than 1MB Previously, users had to explicitly set enableCompression=true in the ZNRecord to enable compression. This meant that the users had to anticipate the size prior hand and set it in the ZNRecord to avoid runtime errors. This patch turns this feature on automatically if the data size is >1MB. You can merge this pull request into a Git repository by running: $ git pull https://github.com/kishoreg/helix auto-compress-znode Alternatively you can review and apply these changes as the patch at: https://github.com/apache/helix/pull/78.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #78 commit c9216c48ea5b900a661fbc1ced7d2f41c8bed38d Author: kishoreg <kisho...@apache.org> Date: 2017-03-28T21:22:14Z Auto compress ZNode that are greater than 1MB --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix issue #77: Adding support to batch ZK callback optionally by setting s...
Github user kishoreg commented on the issue: https://github.com/apache/helix/pull/77 I think it's a good idea to have one shared thread pool. Since we create one callback per helix entity type (idealstate, external view), I thought this is not a big overhead. One problem is for the current state property key which will spawn one thread per participant. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request #77: Adding support to batch ZK callback optionally by se...
GitHub user kishoreg opened a pull request: https://github.com/apache/helix/pull/77 Adding support to batch ZK callback optionally by setting sys var asy⦠Supports batching of zk callbacks. Can be enabled optionally with a system flag (asyncBatchModeEnabled=true). In my test cluster with 1000 resources, I find the number of callbacks processed from ZK reduces from 2.4k to 119. This speeds up the controller and also reduces the load on ZK You can merge this pull request into a Git repository by running: $ git pull https://github.com/kishoreg/helix zk-callback-batching Alternatively you can review and apply these changes as the patch at: https://github.com/apache/helix/pull/77.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #77 commit 384978a2e16ab0f4adb388e32c7e448c77996ca2 Author: kishoreg <kisho...@apache.org> Date: 2017-03-24T17:48:05Z Adding support to batch ZK callback optionally by setting sys var asyncBatchModeEnabled=true --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request #65: [HELIX-651] Add a method in HelixAdmin to set the In...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/65#discussion_r95282034
--- Diff:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java ---
@@ -162,6 +162,20 @@ public InstanceConfig getInstanceConfig(String
clusterName, String instanceName)
}
@Override
+ public boolean setInstanceConfig(String clusterName, String
instanceName,InstanceConfig instanceConfig) {
+String instanceConfigPath =
+PropertyPathConfig.getPath(PropertyType.CONFIGS, clusterName,
ConfigScopeProperty.PARTICIPANT.toString(),
+instanceName);
+if (!_zkClient.exists(instanceConfigPath)) {
+ throw new HelixException("instance" + instanceName + " does not
exist in cluster " + clusterName);
+}
+
+HelixDataAccessor accessor = new ZKHelixDataAccessor(clusterName, new
ZkBaseDataAccessor(_zkClient));
+Builder keyBuilder = accessor.keyBuilder();
+return accessor.setProperty(keyBuilder.instanceConfig(instanceName),
instanceConfig);
--- End diff --
this will override the existing configuration. Helix relies on HELIX_HOST
and HELIX_PORT in this instanceConfig. We have two options here
-- Ensure that these two properties are set. If not, copy them from
previous instanceConfig.
-- Change the method name to updateInstanceConfig and merge the old and new
instanceConfigs. We can do this using
oldInstanceConfig.getRecord().merge(newInstanceConfig.getRecord());
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request #65: [HELIX-651] Add a method in HelixAdmin to set the In...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/65#discussion_r95281496
--- Diff:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixAdmin.java ---
@@ -162,6 +162,20 @@ public InstanceConfig getInstanceConfig(String
clusterName, String instanceName)
}
@Override
+ public boolean setInstanceConfig(String clusterName, String
instanceName,InstanceConfig instanceConfig) {
--- End diff --
Please format using the style sheet.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix issue #58: helix-core: AutoRebalancer should include only numbered sta...
Github user kishoreg commented on the issue:
https://github.com/apache/helix/pull/58
This should fix it. One scenario, where this will result in undesirable
output is when the entire cluster is restarted.
Let's day P was on NODE_0 and NODE_1, we paused the controller, restarted
all nodes and unpaused controller. It's desirable to put P on the same nodes
(NODE_0, NODE_1), but since we remove it from the map, it might reassign it to
another node and this will cause a lot of reshuffling.
Another alternative is to keep the current code but during sorting use the
state priority followed by node names. With this change currentMapping: `{P:
{NODE_0: OFFLINE, NODE_1: ONLINE}}` after sorting should result in `[NODE_1,
NODE_0]` and hence no movement.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix issue #49: New topology-aware auto rebalance strategy and some fixes t...
Github user kishoreg commented on the issue: https://github.com/apache/helix/pull/49 this is great work Lei. Can you please paste the final mapping generated by default strategy v/s crush method. Also when would one chose one over the other. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix issue #49: New topology-aware auto rebalance strategy and some fixes t...
Github user kishoreg commented on the issue: https://github.com/apache/helix/pull/49 lgtm --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request #49: New topology-aware auto rebalance strategy and some ...
Github user kishoreg commented on a diff in the pull request: https://github.com/apache/helix/pull/49#discussion_r76988348 --- Diff: helix-core/src/main/java/org/apache/helix/controller/rebalancer/strategy/CrushRebalanceStrategy.java --- @@ -0,0 +1,174 @@ +package org.apache.helix.controller.rebalancer.strategy; + +/* + * 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 com.google.common.base.Predicate; +import com.google.common.base.Predicates; +import org.apache.helix.HelixException; +import org.apache.helix.ZNRecord; +import org.apache.helix.controller.rebalancer.strategy.crushMapping.CRUSHPlacementAlgorithm; --- End diff -- can we keep the package name as crush instead of crushMapping --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix issue #48: helix-ui improvements
Github user kishoreg commented on the issue: https://github.com/apache/helix/pull/48 LGTM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix issue #48: helix-ui improvements
Github user kishoreg commented on the issue: https://github.com/apache/helix/pull/48 Got it. Will review this. Are you guys using 0.6.x or 0.7.x --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix issue #48: helix-ui improvements
Github user kishoreg commented on the issue: https://github.com/apache/helix/pull/48 @lei-xia can you also review this change. Subbu had a test case https://issues.apache.org/jira/browse/HELIX-631. Will be good to see if this change fixes that as well. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix issue #48: helix-ui improvements
Github user kishoreg commented on the issue: https://github.com/apache/helix/pull/48 this is awesome Greg, can you please create a separate PR for FULL_AUTO bug. I will review the changes tonight. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix issue #46: All task framework changes
Github user kishoreg commented on the issue: https://github.com/apache/helix/pull/46 Lgtm. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: fixed a bug at WriteLock caused by read-delete...
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/44#issuecomment-221774907 LGTM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: A few more task framework improvement
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/41#issuecomment-209564520 Thanks, will apply the change today On Wed, Apr 13, 2016 at 10:46 AM, Lei Xia <notificati...@github.com> wrote: > Rebased to the head. > > â > You are receiving this because you commented. > Reply to this email directly or view it on GitHub > <https://github.com/apache/helix/pull/41#issuecomment-209563974> > --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: A few more task framework improvement
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/41#issuecomment-182482218 Will review this today --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: A few more task framework improvement
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/41#issuecomment-182487167 Looks good, please try to split the changes into smaller RB's. Its very hard to review such a big RB. Can you address the RB comments. I will merge it --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: A few more task framework improvement
Github user kishoreg commented on a diff in the pull request: https://github.com/apache/helix/pull/41#discussion_r52488945 --- Diff: helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java.rej --- @@ -0,0 +1,18 @@ +diff a/helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java b/helix-core/src/main/java/org/apache/helix/controller/stages/BestPossibleStateCalcStage.java (rejected hunks) --- End diff -- why is this file in the source? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: A few more task framework improvement
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/41#discussion_r52489589
--- Diff: helix-core/src/main/java/org/apache/helix/task/JobRebalancer.java
---
@@ -206,9 +205,23 @@ private ResourceAssignment
computeResourceMapping(String jobResource,
TaskAssignmentCalculator taskAssignmentCal =
getAssignmentCalulator(jobCfg);
Set allPartitions =
taskAssignmentCal.getAllTaskPartitions(jobCfg, jobCtx,
workflowConfig, workflowCtx, cache);
+
+if (allPartitions == null || allPartitions.isEmpty()) {
+ // Empty target partitions, mark the job as FAILED.
+ LOG.info(
+ "Missing task partition mapping for job " + jobResource + ",
marked the job as FAILED!");
--- End diff --
should this be warn
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-626] Enable maxRecipient in Criteria
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/43#issuecomment-172105093 Is this intended to be used via curl or via java api?. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: [HELIX-626] Enable maxRecipient in Criteria
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/43#issuecomment-172126774 Then a better api, is to provide a Filter api right. similar to File.list(dir, FileNameFilter) we can have a MessageSendFilter which has accept method that returns boolean within Helix, we can invoke this method and caller has the option to say true or false. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: [Helix-624] Fix NPE in TestMessageService.Test...
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/42#issuecomment-171563196 LGTM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: [Helix-612] Bump up the version of zkClient an...
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/36#issuecomment-170707954 LGTM. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: [HELIX-613] Fix thread leaking problems in Tas...
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/38#issuecomment-154778932 LGTM. Any test case to ensure that there is no leak? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: [HELIX-610] Makes sure TaskStateModel gets pro...
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/35#issuecomment-138792278 This is an interesting change, why cant we do this on the client side? If you have the reference to StateModelFactory, you can call the shutdown method on each and every instance of statemodel --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: [HELIX-609] Added new DataSource values LIVEIN...
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/34#issuecomment-138783773 Thanks LGTM. Will apply the patch --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: Added new DataSource values LIVEINSTANCES and ...
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/34#issuecomment-138435680 Looks good, can you add a simple test case? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: [Helix-606] Add an option in IdealState to all...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/32#discussion_r35688508
--- Diff:
helix-core/src/main/java/org/apache/helix/manager/zk/ZKHelixManager.java ---
@@ -645,7 +645,7 @@ public boolean isLeader() {
if (_helixPropertyStore == null) {
String path = PropertyPathConfig.getPath(PropertyType.PROPERTYSTORE,
_clusterName);
- String fallbackPath = String.format(/%s/%s, _clusterName,
HELIX_PROPERTYSTORE);
--- End diff --
This is kept for backward compatibility, we need this when users upgrade
from 0.6.2 to newer versions
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: Cleaned up ByteBuf usage in IPC
Github user kishoreg commented on the pull request: https://github.com/apache/helix/pull/33#issuecomment-122676485 LGTM --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] helix pull request: [HELIX-599] Support creating/maintaining/routi...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/31#discussion_r33882177
--- Diff: helix-core/src/main/java/org/apache/helix/model/IdealState.java
---
@@ -536,4 +574,16 @@ public boolean isEnabled() {
public void enable(boolean enabled) {
_record.setSimpleField(IdealStateProperty.HELIX_ENABLED.name(),
Boolean.toString(enabled));
}
+
+ /**
+ * Get the mangled IdealState name if resourceGroup is enable.
+ *
+ * @param resourceName
+ * @param resourceTag
+ *
+ * @return
+ */
+ public static String getIdealStateName(String resourceName, String
resourceTag) {
--- End diff --
why do we need this method? This convention can be completely handled on
client side rt?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-599] Support creating/maintaining/routi...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/31#discussion_r33881174
--- Diff:
helix-core/src/main/java/org/apache/helix/spectator/RoutingTableProvider.java
---
@@ -73,6 +75,73 @@ public RoutingTableProvider() {
}
/**
+ * returns the instances for {resource,partition} pair that are in a
specific {state} if
+ * aggregateGrouping is turned on, find all resources belongs to the
given resourceGroupName and
+ * aggregate all partition states from all these resources.
+ *
+ * @param resourceName
+ * @param partitionName
+ * @param state
+ * @param groupingEnabled
+ *
+ * @return empty list if there is no instance in a given state
+ */
+ public ListInstanceConfig getInstances(String resourceName, String
partitionName, String state,
--- End diff --
having boolean here does not make sense. we should problem have
getInstancesForResource and getInstancesForResourceGroup
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-599] Support creating/maintaining/routi...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/31#discussion_r33881022
--- Diff:
helix-core/src/main/java/org/apache/helix/controller/stages/MessageGenerationPhase.java
---
@@ -127,7 +127,9 @@ public void process(ClusterEvent event) throws
Exception {
Message message =
createMessage(manager, resourceName,
partition.getPartitionName(), instanceName,
currentState, nextState,
sessionIdMap.get(instanceName), stateModelDef.getId(),
-resource.getStateModelFactoryname(), bucketSize);
--- End diff --
can we simply pass in the entire resource and createMessage can fetch
required attributes from resource
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-599] Support creating/maintaining/routi...
Github user kishoreg commented on a diff in the pull request:
https://github.com/apache/helix/pull/31#discussion_r33881054
--- Diff:
helix-core/src/main/java/org/apache/helix/controller/stages/MessageGenerationPhase.java
---
@@ -190,7 +192,8 @@ public void process(ClusterEvent event) throws
Exception {
private Message createMessage(HelixManager manager, String resourceName,
String partitionName,
String instanceName, String currentState, String nextState, String
sessionId,
- String stateModelDefName, String stateModelFactoryName, int
bucketSize) {
--- End diff --
This method has too many parameters, we need to just pass in resource or
have a message builder class
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] helix pull request: [HELIX-599] Support creating/maintaining/routi...
Github user kishoreg commented on a diff in the pull request: https://github.com/apache/helix/pull/31#discussion_r33881099 --- Diff: helix-core/src/main/java/org/apache/helix/model/IdealState.java --- @@ -55,7 +55,9 @@ MAX_PARTITIONS_PER_INSTANCE, INSTANCE_GROUP_TAG, REBALANCER_CLASS_NAME, -HELIX_ENABLED +HELIX_ENABLED, +RESOURCE_GROUP_NAME, +RESOURCE_GROUP_ENABLED --- End diff -- Why do we need ResourceGroupEnabled flag? Will things work as expected if there is a resourcegroupName and by default we can set the resourceGroupName to resourceName ? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
