[GitHub] storm pull request #2710: STORM-3099: Extend metrics on supervisor, workers ...

[revans2 <gi...@git.apache.org>]

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

    https://github.com/apache/storm/pull/2710#discussion_r207576736
  
    --- Diff: storm-client/src/jvm/org/apache/storm/daemon/supervisor/ClientSupervisorUtils.java ---
    @@ -28,11 +29,14 @@
     import org.apache.storm.shade.org.apache.commons.lang.StringUtils;
     import org.apache.storm.utils.ConfigUtils;
     import org.apache.storm.utils.ObjectReader;
    +import org.apache.storm.utils.ShellUtils;
     import org.apache.storm.utils.Utils;
     import org.slf4j.Logger;
     import org.slf4j.LoggerFactory;
     
     public class ClientSupervisorUtils {
    +    public static final Meter numWorkerLaunchExceptions = ShellUtils.numShellExceptions;
    --- End diff --
    
    This all feels a bit too confusing to me.
    
    `ShellUtils.numShellExceptions` is static and pulled in from multiple different locations, but only registered once in the supervisor. I personally would rather see it where `ClientSupervisorUtils` registers the Meter, as it is tied to the supervisor having it do it here is fine.
    
    For ShellUtils, I would like to see it not have a static meter, but instead optionally pass a meter in when calling run, or perhaps optionally include it in the constructor. 


---