Ethanlm commented on a change in pull request #3213: [STORM-3588] add GenericResourceAwareSchedulingPriorityStrategy to accommodate generic resource in grading topologies URL: https://github.com/apache/storm/pull/3213#discussion_r384785673
########## File path: storm-server/src/test/java/org/apache/storm/scheduler/resource/strategies/priority/TestGenericResourceAwareSchedulingPriorityStrategy.java ########## @@ -0,0 +1,216 @@ +/* + * 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. + */ + +package org.apache.storm.scheduler.resource.strategies.priority; + +import org.apache.storm.Config; +import org.apache.storm.DaemonConfig; +import org.apache.storm.metric.StormMetricsRegistry; +import org.apache.storm.scheduler.Cluster; +import org.apache.storm.scheduler.INimbus; +import org.apache.storm.scheduler.IScheduler; +import org.apache.storm.scheduler.SupervisorDetails; +import org.apache.storm.scheduler.Topologies; +import org.apache.storm.scheduler.resource.ResourceAwareScheduler; +import org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler; +import org.apache.storm.scheduler.resource.normalization.ResourceMetrics; +import org.apache.storm.scheduler.resource.strategies.eviction.TestDefaultEvictionStrategy; +import org.apache.storm.utils.Time; +import org.junit.After; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +import static org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler.addTopologies; +import static org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler.assertTopologiesBeenEvicted; +import static org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler.assertTopologiesFullyScheduled; +import static org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler.assertTopologiesNotBeenEvicted; +import static org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler.assertTopologiesNotScheduled; +import static org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler.createGrasClusterConfig; +import static org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler.genSupervisors; +import static org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler.genTopology; +import static org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler.userRes; +import static org.apache.storm.scheduler.resource.TestUtilsForResourceAwareScheduler.userResourcePool; + +public class TestGenericResourceAwareSchedulingPriorityStrategy { + + private static final Logger LOG = LoggerFactory.getLogger(TestDefaultEvictionStrategy.class); + private static int currentTime = Time.currentTimeSecs(); + private static IScheduler scheduler = null; + + @After + public void cleanup() { + if (scheduler != null) { + scheduler.cleanup(); + scheduler = null; + } + } + + /* + * DefaultSchedulingPriorityStrategy will not evict topo as long as the resources request can be met + * + * Ethan asks for heavy cpu and memory while Rui asks for little cpu and memory but heave generic resource + * Since Rui's all types of resources request can be met, no eviction will happend + */ + @Test + public void testDefaultSchedulingPriorityStrategyNotEvicting() { + Map<String, Double> requestedgenericResourcesMap = new HashMap<>(); + requestedgenericResourcesMap.put("generic.resource.1", 40.0); + // Use full memory and cpu of the cluster capacity + Config ruiConf = createGrasClusterConfig(20, 50, 50, null, requestedgenericResourcesMap); + Config ethanConf = createGrasClusterConfig(80, 400, 500, null, Collections.emptyMap()); + Topologies topologies = new Topologies( + genTopology("ethan-topo-1", ethanConf, 1, 0, 1, 0, currentTime - 2, 10, "ethan"), + genTopology("ethan-topo-2", ethanConf, 1, 0, 1, 0, currentTime - 2, 20, "ethan"), + genTopology("ethan-topo-3", ethanConf, 1, 0, 1, 0, currentTime - 2, 28, "ethan"), + genTopology("ethan-topo-4", ethanConf, 1, 0, 1, 0, currentTime - 2, 29, "ethan")); + + Topologies withNewTopo = addTopologies(topologies, + genTopology("rui-topo-1", ruiConf, 1, 0, 4, 0, currentTime - 2, 10, "rui")); + + Config config = mkClusterConfig(DefaultSchedulingPriorityStrategy.class.getName()); + Cluster cluster = mkTestCluster(topologies, config); + scheduler = new ResourceAwareScheduler(); + scheduler.prepare(config); + scheduler.schedule(topologies, cluster); + + assertTopologiesFullyScheduled(cluster, "ethan-topo-1", "ethan-topo-2", "ethan-topo-3", "ethan-topo-4"); + + + cluster = new Cluster(cluster, withNewTopo); + scheduler.schedule(withNewTopo, cluster); + + assertTopologiesFullyScheduled(cluster, "ethan-topo-1", "ethan-topo-2", "ethan-topo-3", "ethan-topo-4"); + assertTopologiesNotBeenEvicted(cluster, (ResourceAwareScheduler) scheduler, "ethan-topo-1", "ethan-topo-2", "ethan-topo-3", "ethan-topo-4"); + assertTopologiesFullyScheduled(cluster, "rui-topo-1"); + } + + /* + * DefaultSchedulingPriorityStrategy does not take generic resources into account when calculating score + * So even if a user is requesting a lot of generic resources other than CPU and memory, scheduler will still score it very low and kick out other topologies + * + * Ethan asks for medium cpu and memory while Rui asks for little cpu and memory but heave generic resource + * However, Rui's generic request can not be met and default scoring system is not taking generic resources into account, + * so the score of Rui's new topo will be much lower than all Ethan's topos'. + * Then all Ethan's topo will be evicted in trying to make rooms for Rui. + */ + @Test + public void testDefaultSchedulingPriorityStrategyEvicting() { + Map<String, Double> requestedgenericResourcesMap = new HashMap<>(); + requestedgenericResourcesMap.put("generic.resource.1", 40.0); + Config ruiConf = createGrasClusterConfig(10, 10, 10, null, requestedgenericResourcesMap); + Config ethanConf = createGrasClusterConfig(60, 200, 300, null, Collections.emptyMap()); + Topologies topologies = new Topologies( + genTopology("ethan-topo-1", ethanConf, 1, 0, 1, 0, currentTime - 2, 10, "ethan"), + genTopology("ethan-topo-2", ethanConf, 1, 0, 1, 0, currentTime - 2, 20, "ethan"), + genTopology("ethan-topo-3", ethanConf, 1, 0, 1, 0, currentTime - 2, 28, "ethan"), + genTopology("ethan-topo-4", ethanConf, 1, 0, 1, 0, currentTime - 2, 29, "ethan")); + + Topologies withNewTopo = addTopologies(topologies, + genTopology("rui-topo-1", ruiConf, 1, 0, 5, 0, currentTime - 2, 10, "rui")); + + Config config = mkClusterConfig(DefaultSchedulingPriorityStrategy.class.getName()); + Cluster cluster = mkTestCluster(topologies, config); + scheduler = new ResourceAwareScheduler(); + scheduler.prepare(config); + scheduler.schedule(topologies, cluster); + + assertTopologiesFullyScheduled(cluster, "ethan-topo-1", "ethan-topo-2", "ethan-topo-3", "ethan-topo-4"); + + + cluster = new Cluster(cluster, withNewTopo); + scheduler.schedule(withNewTopo, cluster); + + assertTopologiesFullyScheduled(cluster, "ethan-topo-1", "ethan-topo-2", "ethan-topo-3", "ethan-topo-4"); + assertTopologiesBeenEvicted(cluster, (ResourceAwareScheduler) scheduler, "ethan-topo-1", "ethan-topo-2", "ethan-topo-3", "ethan-topo-4"); + assertTopologiesNotScheduled(cluster, "rui-topo-1"); + } + + /* + * GenericResourceAwareSchedulingPriorityStrategy extend scoring formula to accommodate generic resources + * + * Same setting as the above test, but this time, new scoring system is taking generic resources into account, Review comment: For `as the above test` , maybe it's better to refer to the test case directly. If anyone moves the order of these unit tests, this comment can be confusing ---------------------------------------------------------------- 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: [email protected] With regards, Apache Git Services
