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