[ 
https://issues.apache.org/jira/browse/HBASE-4050?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13413535#comment-13413535
 ] 

stack commented on HBASE-4050:
------------------------------

I suppose you don't need to set test size annotation on below because 
annotations are not a dependency when this is built:

{code}
+public class ReplicationMetricsSourceFactoryTest {
{code}

Does BaseMetricsSource not implement MetricsSource?

{code}
+public class BaseMetricsSourceImpl implements BaseMetricsSource, MetricsSource 
{
{code}

These need to be this accessible:

{code}
+  public ConcurrentMap<String, MetricMutableGaugeLong>
+      gauges = new ConcurrentHashMap<String, MetricMutableGaugeLong>();
+  public ConcurrentMap<String, MetricMutableCounterLong> counters =
+      new ConcurrentHashMap<String, MetricMutableCounterLong>();
+
+  protected String metricsContext;
+  protected String metricsName;
+  protected String metricsDescription;
{code}

(I see above twice)

The stuff below where we have a static boolean and in constructor we test 
something already created could be a PITA in minihbase setups?  Does it have to 
be static?  Aren't we slinging singletons here anyways?  (The singletons are ok 
in minihbasecontext too?):

{code}
+    if (!hasInited) {
+      //Not too worried about mutli-threaded here as all it does is spam the 
logs.
+      hasInited = true;
+      DefaultMetricsSystem.initialize(HBASE_METRICS_SYSTEM_NAME);
+    }
{code}

'hasInited' is name of a method that tests 'inited' variable... suggest 
changing its name.

What about that jmx mess registering metrics in tests?  The exception saying 
metrics already registered because we have more than one daemon in the one jvm. 
 We still have that issue here?

You wanted to complete this: "+/** BaseClass for */"

Another class has no class comments though has the comment delimiters.

Do we have to have metrics2 package?  Can this new stuff be in the metrics 
package?

I thought I saw a patch where you'd renamed the properties file to what LarsG 
suggested?

You seem to have made it so we do not need to have a metrics2 in hbase... thats 
great... but in the properties file I see:

{code}
+# See package.html for org.apache.hadoop.metrics2 for details
+
+*.sink.file.class=org.apache.hadoop.metrics2.sink.FileSink
{code}

Is that just old stuff?

Good stuff Elliott.  I'd be up for committing this and then doing other stuff 
in other issues.


                
> Update HBase metrics framework to metrics2 framework
> ----------------------------------------------------
>
>                 Key: HBASE-4050
>                 URL: https://issues.apache.org/jira/browse/HBASE-4050
>             Project: HBase
>          Issue Type: New Feature
>          Components: metrics
>    Affects Versions: 0.90.4
>         Environment: Java 6
>            Reporter: Eric Yang
>            Assignee: Alex Baranau
>            Priority: Critical
>             Fix For: 0.96.0
>
>         Attachments: 4050-metrics-v2.patch, 4050-metrics-v3.patch, 
> HBASE-4050-0.patch, HBASE-4050-1.patch, HBASE-4050-2.patch, 
> HBASE-4050-3.patch, HBASE-4050-5.patch, HBASE-4050-6.patch, 
> HBASE-4050-7.patch, HBASE-4050.patch
>
>
> Metrics Framework has been marked deprecated in Hadoop 0.20.203+ and 0.22+, 
> and it might get removed in future Hadoop release.  Hence, HBase needs to 
> revise the dependency of MetricsContext to use Metrics2 framework.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to