Hadoop Common
  1. Hadoop Common
  2. HADOOP-3470

Bad coding style: The member fields in org.apache.hadoop.ipc.metrics.RpcMetrics are public

    Details

    • Type: Improvement Improvement
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: metrics
    • Labels:
      None

      Description

      In org.apache.hadoop.ipc.metrics.RpcMetrics,

      //the following are member fields
        public MetricsTimeVaryingRate rpcQueueTime = new MetricsTimeVaryingRate("RpcQueueTime");
        public MetricsTimeVaryingRate rpcProcessingTime = new MetricsTimeVaryingRate("RpcProcessingTime");
      
        public Map <String, MetricsTimeVaryingRate> metricsList = Collections.synchronizedMap(new HashMap<String, MetricsTimeVaryingRate>());
      

      Then, the fields are accessed directly in other classes. For example, org.apache.hadoop.ipc.RPC.Server.call(...)

      ...
      	MetricsTimeVaryingRate m = rpcMetrics.metricsList.get(call.getMethodName());
      
      	if (m != null) {
      		m.inc(processingTime);
      	}
      	else {
      		rpcMetrics.metricsList.put(call.getMethodName(), new MetricsTimeVaryingRate(call.getMethodName()));
      		m = rpcMetrics.metricsList.get(call.getMethodName());
      		m.inc(processingTime);
      	}
      

        Issue Links

          Activity

          Tsz Wo Nicholas Sze created issue -
          Tsz Wo Nicholas Sze made changes -
          Field Original Value New Value
          Link This issue incorporates HADOOP-4838 [ HADOOP-4838 ]
          Hide
          Sanjay Radia added a comment -

          Generally member fields should be accessed via getters and setter.
          In this particular case the purpose of the metrics holder class (such as RPCMetrics or NameNodeMetrics, etc is to be place holder
          of the a list of metrics variables that are then accessed in various parts of the code to change the counters.

          Forcing a set of new methods per metrics (since a metrics may have multiple operations) is very painful and will make it
          harder to add new metrics. Adding metrics should be simple"

          • declare metrics variable (eg. counter)
          • write code to change the metrics variable (e.g inc the counter).

          (btw HADOOP-4838 simplifies metrics so that only the above two steps are needed.)

          I believe the metrics variables are one of the exception to style rule of using getters and setter for fields.
          (Note HADOOP4848 has added a registry and hence it would be easy to have the callers use that map but the cost of that
          would be too much for changing counters (ie a map lookup operation per counter increment!!)

          Show
          Sanjay Radia added a comment - Generally member fields should be accessed via getters and setter. In this particular case the purpose of the metrics holder class (such as RPCMetrics or NameNodeMetrics, etc is to be place holder of the a list of metrics variables that are then accessed in various parts of the code to change the counters. Forcing a set of new methods per metrics (since a metrics may have multiple operations) is very painful and will make it harder to add new metrics. Adding metrics should be simple" declare metrics variable (eg. counter) write code to change the metrics variable (e.g inc the counter). (btw HADOOP-4838 simplifies metrics so that only the above two steps are needed.) I believe the metrics variables are one of the exception to style rule of using getters and setter for fields. (Note HADOOP4848 has added a registry and hence it would be easy to have the callers use that map but the cost of that would be too much for changing counters (ie a map lookup operation per counter increment!!)
          Hide
          Tsz Wo Nicholas Sze added a comment -

          How about making all these fields final?

          Show
          Tsz Wo Nicholas Sze added a comment - How about making all these fields final?
          Tsz Wo Nicholas Sze made changes -
          Link This issue is related to HADOOP-5714 [ HADOOP-5714 ]
          Hide
          Allen Wittenauer added a comment -

          Closing this as won't fix given the metrics system has been replaced.

          Show
          Allen Wittenauer added a comment - Closing this as won't fix given the metrics system has been replaced.
          Allen Wittenauer made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Resolution Won't Fix [ 2 ]
          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Resolved Resolved
          2239d 10h 46m 1 Allen Wittenauer 18/Jul/14 06:25

            People

            • Assignee:
              Unassigned
              Reporter:
              Tsz Wo Nicholas Sze
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development