Github user revans2 commented on the pull request:

    https://github.com/apache/storm/pull/1147#issuecomment-191986179
  
    I am going through StatsUtil.java and it is more of the same thing 
everywhere.  The code is a literal translation of the clojure code.  The input 
APIs in most cases are clojure specific, but many of these functions actually 
return thrift object already.  For example `stats/agg-comp-execs-stats` returns 
an instance of `ComponentPageInfo`.  I would prefer to see us have a shim that 
will translate the clojure maps, etc into java objects that would be a better 
java API/state when we translate the clojure code that is calling into it.
    
    For example 
    ```
    (defn agg-comp-execs-stats
      "Aggregate various executor statistics for a component from the given
      heartbeats."
      [exec->host+port
       task->component
       beats
       window
       include-sys?
       topology-id
       topology
       component-id]
    
    ```
    
    was translated into
    
    ```
    public static TopologyPageInfo aggTopoExecsStats(
      String topologyId,
      Map exec2nodePort,
      Map task2component,
      Map beats,
      StormTopology topology,
      String window,
      boolean includeSys,
      IStormClusterState clusterState);
    ```
    
    but if we expand the signature a bit more it really looks like
    
    ```
    public static TopologyPageInfo aggTopoExecsStats(
      String topologyId,
      Map<List<Number>/*low exec, high exec*/,List<Object>/*String host, Number 
port*/ > exec2nodePort,
      Map<Integer, String> task2component,
      Map beats<List<Number>/*low exec, high exec*/, Map<Keyword, 
Object>/*Clojure map keys :stats, :uptime, :time-secs, good luck*/ >,
      StormTopology topology,
      String window,
      boolean includeSys,
      IStormClusterState clusterState);
    
    ```
    
    If we want a more java like API with classes instead of maps/Lists it 
should probably be something more like
    
    ```
    public class HostPort {
        public final String host;
        public final int port;
        ...
    };
    
    public static TopologyPageInfo aggTopoExecsStats(
        String topologyId, 
        Map<ExecutorInfo, HostPort> exec2nodePort,
        Map<Integer, String> task2component,
        Map beats<ExecutorInfo, ExecutorBeat>, 
        StormTopology topology,
        String window,
        boolean includeSys,
        IStormClusterState clusterState);
    ```
    
    All of the public facing APIs need this type of a translation to happen.  
If you want me to go through all of them and list what I think they should be I 
can.


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