[STORM-2201] Add dynamic scheduler configuration loading. #1785

ppoulosk · 2016-11-17T16:02:09Z

This adds an interface and two implementations, one that will load from a local file, and another
that will load a config from artifactory.

It also modifies the ResourceAwareScheduler and Multitenant schedulers to have a plugin interface and configuration entries for the plugin.

Unit tests are also added for the implementations.

revans2

Overall it looks good, and it is nice to have the option to load data dynamically from a central location for resource management.

Needs documentation and possibly a bit of refactoring.

It would also be really nice (and can be in a follow on JIRA) to have a loader that can just point to a single URL on an HTTP server. In place update instead of needing to list the directory in artifactory.

revans2 · 2016-11-21T16:27:44Z

storm-core/src/jvm/org/apache/storm/Config.java

+     * A plugin that should load the user pools for the resource aware scheduler
+     */
+    @isString
+    public static final String RESOURCE_AWARE_SCHEDULER_USER_POOLS_LOADER = "resource.aware.scheduler.user.pools.loader";


If this is intended to be a subclass of something, it might be good to use @ isImplementationOfClass instead of @isString

Specifically @isImplementationOfClass(implementsClass = org.apache.storm.scheduler.utils.IConfigLoader.class)

Yup that is what @isImplementationOfClass is for. We should use it

revans2 · 2016-11-21T16:30:09Z

storm-core/src/jvm/org/apache/storm/scheduler/multitenant/MultitenantScheduler.java

+        if (ret != null) {
+            return ret;
+        } else {
+            LOG.debug("Config loader returned null");


Should this be a warn instead of a debug?

revans2 · 2016-11-21T16:31:42Z

storm-core/src/jvm/org/apache/storm/scheduler/utils/ArtifactoryConfigLoader.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ArtifactoryConfigLoader implements IConfigLoader {


Could you add some javadocs here? and add something to the docs to describe this new feature, how it works and how to use it?

revans2 · 2016-11-21T16:34:02Z

storm-core/src/jvm/org/apache/storm/scheduler/utils/ArtifactoryConfigLoader.java

+    private int _timeoutSeconds = 10;
+    private Map _lastReturnedValue;
+
+    private static class OurResponseHandler implements ResponseHandler<String> {


Perhaps we can name this a bit better. It appears to validate that the response code was in the 2xx range and then convert the body to a string. Maybe GETStringResponseHandler would be better?

revans2 · 2016-11-21T16:38:50Z

storm-core/src/jvm/org/apache/storm/scheduler/utils/ArtifactoryConfigLoader.java

+        if (api != null) {
+            builder.setPath(_baseDirectory + api + artifact);
+        } else {
+            builder.setPath(_baseDirectory + artifact);


Do we want to filter out double back slashes? "//". If I remember correctly I think I have had issues with artifactory and too many / characters, almost like it is running jetty behind the scenes.

If we filter out double backslashes we can also add in extra ones to be sure we have a slash between api and artifact instead of just assuming there is one.

revans2 · 2016-11-21T16:41:47Z

storm-core/src/jvm/org/apache/storm/scheduler/utils/IConfigLoader.java

+     */
+    void prepare(Map conf);
+
+    Map load();


Can we make this at least a little generic? Something like Map<?,?> instead of just Map

revans2 · 2016-11-21T16:42:06Z

storm-core/src/jvm/org/apache/storm/scheduler/utils/IConfigLoader.java

+     * 
+     * See implementing classes for configuration details.
+     */
+    void prepare(Map conf);


For conf can be please add the generics? Map<String, Object>

revans2 · 2016-11-21T16:44:06Z

storm-core/src/jvm/org/apache/storm/scheduler/utils/SchedulerUtils.java

+        return null;
+    }
+
+    public static Map loadYamlFromFile(File file) {


Can we please add is some generics to the Map.

Can we also add in some javadocs or possibly change the name? It is not obvious from the name or really anything else that it will eat exceptions and return null.

revans2 · 2016-11-21T16:45:30Z

storm-core/src/jvm/org/apache/storm/scheduler/utils/SchedulerUtils.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class SchedulerUtils {


Now that we are on java 8 it might be good to move these static methods into the IConfigLoader interface. in java 8 static methods can be part of an interface.

ppoulosk · 2016-12-05T15:24:34Z

@revans2, Thanks for the review. I've merged in a commit to fix the issues you have pointed out.

abellina · 2016-12-14T15:51:21Z

@ppoulosk can we add a document as @revans2 suggested? A markdown file should do it.

ppoulosk · 2016-12-16T15:44:08Z

Markdown added for interface and describing the configuration of implementations.

abellina · 2016-12-16T16:05:08Z

thanks @ppoulosk, will do another pass today.

ppoulosk · 2017-01-06T16:15:34Z

@abellina @revans2 Could you look at the re-work on this pull?

revans2

Overall it looks good, I have one minor comment about some wording in the documentation.

revans2 · 2017-01-06T16:25:47Z

docs/IConfigLoader.md

+
+
+### Introduction
+IConfigLoader is an interface designed to allow way to dynamically load scheduler resource constraints into scheduler implementations. Currently, the MultiTenant scheduler uses this interface to dynamically load the number of isolated nodes a given user has been guaranteed, and the ResoureAwareScheduler uses the interface to dynamically load per user resource guarantees.


Perhaps "designed to allow dynamic loading of scheduler resource constraints"

Much better wording. Thanks.

abellina

@ppoulosk thanks for adding markdown file. I had some more small comments.

abellina · 2017-01-06T16:40:33Z

pom.xml

@@ -220,6 +220,7 @@
        <java_jmx.version>0.3.1</java_jmx.version>
        <compojure.version>1.1.9</compojure.version>
        <hiccup.version>0.3.6</hiccup.version>
+        <commons-logging.version>1.2</commons-logging.version>


why are we adding commons-logging? I think I am missing something.

Fixing this.

abellina · 2017-01-06T16:52:39Z

storm-core/src/jvm/org/apache/storm/scheduler/resource/ResourceAwareScheduler.java

        Map<String, Map<String, Double>> ret = new HashMap<String, Map<String, Double>>();

        if (raw != null) {
-            for (Map.Entry<String, Map<String, Number>> userPoolEntry : ((Map<String, Map<String, Number>>) raw).entrySet()) {
+            for (Map.Entry<String, Map<String, Number>> userPoolEntry : raw.entrySet()) {


abellina · 2017-01-06T16:56:12Z

storm-core/src/jvm/org/apache/storm/scheduler/utils/ArtifactoryConfigLoader.java

+
+        String path = null;
+        if (api != null) {
+            path = _baseDirectory + "/" + api + "/" + artifact;


instead of "/" here I'd use the .path method in URIBuilder to build up the path.

there is no .path method in URIbuilder. It's in javax.ws.rs.core.UriBuilder, which isn't pulled into storm anywhere. I'd prefer to leave it as it, but will change and pull in new UriBuilder class if there is a consensus to do so. @revans2?

abellina · 2017-01-06T16:56:24Z

storm-core/src/jvm/org/apache/storm/scheduler/utils/ArtifactoryConfigLoader.java

+        }
+
+        // Get rid of multiple '/' in url
+        path = path.replaceAll("/[/]+", "/");


if you do above, you may not need this one.

HeartSaVioR · 2017-03-08T20:54:29Z

@ppoulosk Could you address @revans2 and @abellina review comments?

ppoulosk · 2017-03-08T21:06:23Z

Will get to this today or tomorrow. Sorry this slipped off of my radar.

two implementations, one that will load from a local file, and another that will load a config from artifactory. Merge pull request apache#807 from ppoulosk/YSTORM-3095-redux [YSTORM-3095] [YSTORM-3779] Re-merge and fix Artifactory scheduler plugins Move to org.apache

HeartSaVioR · 2017-07-10T02:11:33Z

@ppoulosk Any updates?
@Ethanlm I guess you're taking over @ppoulosk work. Could you also consider taking over this work as well if possible?
This patch now addresses all comments from @revans2 and @abellina so seems like close to ready to merge. So if you and @ppoulosk don't want to continue this work, I'll just pick this and rebase.

Ethanlm · 2017-07-10T03:49:55Z

@HeartSaVioR I am willing to do it but I will need to ask @revans2 for his opinion. Thanks

Ethanlm · 2017-07-10T13:36:38Z

@HeartSaVioR I asked @revans2 . I will start to do it.

Ethanlm · 2017-07-10T17:24:52Z

I copied the code and submitted a new pull request. Also fixed some checkstyle issues. #2199

We are closing stale Pull Requests to make the list more manageable. Please re-open any Pull Request that has been closed in error. Closes apache#608 Closes apache#639 Closes apache#640 Closes apache#648 Closes apache#662 Closes apache#668 Closes apache#692 Closes apache#705 Closes apache#724 Closes apache#728 Closes apache#730 Closes apache#753 Closes apache#803 Closes apache#854 Closes apache#922 Closes apache#986 Closes apache#992 Closes apache#1019 Closes apache#1040 Closes apache#1041 Closes apache#1043 Closes apache#1046 Closes apache#1051 Closes apache#1078 Closes apache#1146 Closes apache#1164 Closes apache#1165 Closes apache#1178 Closes apache#1213 Closes apache#1225 Closes apache#1258 Closes apache#1259 Closes apache#1268 Closes apache#1272 Closes apache#1277 Closes apache#1278 Closes apache#1288 Closes apache#1296 Closes apache#1328 Closes apache#1342 Closes apache#1353 Closes apache#1370 Closes apache#1376 Closes apache#1391 Closes apache#1395 Closes apache#1399 Closes apache#1406 Closes apache#1410 Closes apache#1422 Closes apache#1427 Closes apache#1443 Closes apache#1462 Closes apache#1468 Closes apache#1483 Closes apache#1506 Closes apache#1509 Closes apache#1515 Closes apache#1520 Closes apache#1521 Closes apache#1525 Closes apache#1527 Closes apache#1544 Closes apache#1550 Closes apache#1566 Closes apache#1569 Closes apache#1570 Closes apache#1575 Closes apache#1580 Closes apache#1584 Closes apache#1591 Closes apache#1600 Closes apache#1611 Closes apache#1613 Closes apache#1639 Closes apache#1703 Closes apache#1711 Closes apache#1719 Closes apache#1737 Closes apache#1760 Closes apache#1767 Closes apache#1768 Closes apache#1785 Closes apache#1799 Closes apache#1822 Closes apache#1824 Closes apache#1844 Closes apache#1874 Closes apache#1918 Closes apache#1928 Closes apache#1937 Closes apache#1942 Closes apache#1951 Closes apache#1957 Closes apache#1963 Closes apache#1964 Closes apache#1965 Closes apache#1967 Closes apache#1968 Closes apache#1971 Closes apache#1985 Closes apache#1986 Closes apache#1998 Closes apache#2031 Closes apache#2032 Closes apache#2071 Closes apache#2076 Closes apache#2108 Closes apache#2119 Closes apache#2128 Closes apache#2142 Closes apache#2174 Closes apache#2206 Closes apache#2297 Closes apache#2322 Closes apache#2332 Closes apache#2341 Closes apache#2377 Closes apache#2414 Closes apache#2469

revans2 reviewed Nov 21, 2016

View reviewed changes

revans2 reviewed Jan 6, 2017

View reviewed changes

abellina reviewed Jan 6, 2017

View reviewed changes

Paul Poulosky added 3 commits March 30, 2017 09:35

Rework for code review from evans

7242b6d

Add md file

135ac95

ppoulosk force-pushed the STORM-2201 branch from 4793802 to 135ac95 Compare March 30, 2017 14:36

Address review comments

63abe63

Ethanlm mentioned this pull request Jul 10, 2017

[STORM-2201] Add dynamic scheduler configuration loading #2199

Merged

d2r mentioned this pull request Oct 16, 2018

STORM-3250: Closes Pull Requests unchanged in 2018 #2880

Merged

asfgit closed this in #2880 Oct 22, 2018



		### Introduction
		IConfigLoader is an interface designed to allow way to dynamically load scheduler resource constraints into scheduler implementations. Currently, the MultiTenant scheduler uses this interface to dynamically load the number of isolated nodes a given user has been guaranteed, and the ResoureAwareScheduler uses the interface to dynamically load per user resource guarantees.

[STORM-2201] Add dynamic scheduler configuration loading. #1785

[STORM-2201] Add dynamic scheduler configuration loading. #1785

Conversation

ppoulosk commented Nov 17, 2016

revans2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppoulosk commented Dec 5, 2016

abellina commented Dec 14, 2016

ppoulosk commented Dec 16, 2016

abellina commented Dec 16, 2016

ppoulosk commented Jan 6, 2017

revans2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abellina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeartSaVioR commented Mar 8, 2017

ppoulosk commented Mar 8, 2017

HeartSaVioR commented Jul 10, 2017

Ethanlm commented Jul 10, 2017

Ethanlm commented Jul 10, 2017

Ethanlm commented Jul 10, 2017