Github user d2r commented on a diff in the pull request:

    https://github.com/apache/storm/pull/1053#discussion_r53109116
  
    --- Diff: storm-core/src/jvm/org/apache/storm/Config.java ---
    @@ -2194,6 +2195,73 @@
         @isString
         public static final Object CLIENT_JAR_TRANSFORMER = 
"client.jartransformer.class";
     
    +
    +    /**
    +     * The plugin to be used for resource isolation
    +     */
    +    @isImplementationOfClass(implementsClass = 
ResourceIsolationInterface.class)
    +    public static final Object STORM_RESOURCE_ISOLATION_PLUGIN = 
"storm.resource.isolation.plugin";
    +
    +    /**
    +     * CGroup Setting below
    +     */
    +
    +    /**
    +     * root directory of the storm cgroup hierarchy
    +     */
    +    @isString
    +    public static final Object STORM_CGROUP_HIERARCHY_DIR = 
"storm.cgroup.hierarchy.dir";
    +
    +    /**
    +     * resources to to be controlled by cgroups
    +     */
    +    @isStringList
    +    public static final Object STORM_CGROUP_RESOURCES = 
"storm.cgroup.resources";
    +
    +    /**
    +     * name for the cgroup hierarchy
    +     */
    +    @isString
    +    public static final Object STORM_CGROUP_HIERARCHY_NAME = 
"storm.cgroup.hierarchy.name";
    +
    +    /**
    +     * flag to determine whether to use a resource isolation plugin
    +     * Also determines whether the unit tests for cgroup runs.
    +     * If storm.resource.isolation.plugin.enable is set to false the unit 
tests for cgroups will not run
    +     */
    +    @isBoolean
    +    public static final String STORM_RESOURCE_ISOLATION_PLUGIN_ENABLE = 
"storm.resource.isolation.plugin.enable";
    +
    +    /**
    +     * root directory for cgoups
    +     */
    +    @isString
    +    public static String STORM_SUPERVISOR_CGROUP_ROOTDIR = 
"storm.supervisor.cgroup.rootdir";
    +
    +    /**
    +     * the manually set memory limit (in MB) for each CGroup on supervisor 
node
    +     */
    +    @isPositiveNumber
    +    public static String STORM_WORKER_CGROUP_MEMORY_MB_LIMIT = 
"storm.worker.cgroup.memory.mb.limit";
    +
    +    /**
    +     * the manually set cpu share for each CGroup on supervisor node
    +     */
    +    @isPositiveNumber
    +    public static String STORM_WORKER_CGROUP_CPU_LIMIT = 
"storm.worker.cgroup.cpu.limit";
    +
    +    /**
    +     * full path to cgexec command
    +     */
    +    @isString
    +    public static String STORM_CGROUP_CGEXEC_CMD = 
"storm.cgroup.cgexec.cmd";
    +
    +    /**
    +     * The amount of memory a worker can exceed its allocation before 
cgroup will kill it
    +     */
    +    @isPositiveNumber
    +    public static String STORM_CGROUP_MEMORY_MB_LIMIT_TOLERANCE_MARGIN = 
"storm.cgroup.memory.mb.limit.tolerance.margin";
    --- End diff --
    
    This is not exactly what I had in mind from my review comment. Now the Java 
name is consistent with the config name, but if we look at the other config 
definitions, there is also a consistency in the units of the value being 
appended to the name, like `topology.worker.max.heap.size.mb`. This PR does 
something different with `storm.worker.cgroup.memory.mb.limit` and 
`storm.cgroup.memory.mb.limit.tolerance.margin` where the unit is somewhere in 
the middle of the config.  I think it is better to be consistent with the other 
existing configs.


---
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.
---

Reply via email to