[
https://issues.apache.org/jira/browse/HADOOP-10161?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13852613#comment-13852613
]
stack commented on HADOOP-10161:
--------------------------------
Patch makes good sense excepting the below nits. I do not have a ganglia
instance to check this patch against. Can you upload evidence it works for you
in your deploy?
Why you add the hadoop1 and hadoop2 metrics files to this JIRA? Are these
supposed to be checked in along w/ the patch? If so, why not just include the
changes to these files in the patch itself? (Mention of dmax in the
.properties file would be good documentation that dmax can now be twiddled
with).
+ the unorthodox formatting. See the code around your changes. See how it
has spaces between operators; it does not squash statements altogether without
spacing as in this example '+ if ((offset+4)>message.length()){' ... and
then also how there is NO space after and before parens as in this: '+
if ( binaryStr.indexOf(dmx_str) >= 0 ){' There is also this practice where
you have a space before the semicolon: '"units_default" ;' This is all
unorthodox.
+ is there nothing in hadoop that will do the bytes to int for you in str_int
or you have to do it in here in the test? Ditto on xdr_int. Or, are we
mocking ganglia facility here? If so, say so in a method comment else
subsequent readers will be scratching their head asking the same questions I do
above. I am not following either why xdr_int is defined in tests but used in
main code (unless this is a mock?)
+ Why the check against 133 here: '+ if (133 !=
str_int(binaryStr,0)){' ?
+ Is there no tmax in the metrics2 context. You seem to adjust it in metrics1
but do nothing for metrics2 -- maybe that is fine... just asking.
+ These public strings could do with a bit of javadoc I'd say:
+ public static final String CONTEXT_DMAX_PROPERTY = "dmax_default" ;
+ public static final String CONTEXT_TMAX_PROPERTY = "tmax_default" ;
+ public static final String CONTEXT_SLOP_PROPERTY = "slop_default" ;
+ public static final String CONTEXT_UNITS_PROPERTY = "units_default" ;
While dmax/tmax probably don't need it, I'm not sure what SLOP is about and
what units could be explained too.
Do you mean to copy the SLOP config into a 'slopeString' <-- extra 'e' after
slop:
+ String slopeString = conf.getString(CONTEXT_SLOP_PROPERTY);
Thanks.
> Add a method to change the default value of dmax in hadoop.properties
> ---------------------------------------------------------------------
>
> Key: HADOOP-10161
> URL: https://issues.apache.org/jira/browse/HADOOP-10161
> Project: Hadoop Common
> Issue Type: Improvement
> Components: metrics
> Affects Versions: 2.2.0
> Reporter: Yang He
> Attachments: HADOOP-10161_0_20131211.patch,
> HADOOP-10161_1_20131217.patch, HADOOP-10161_DESCRIPTION,
> hadoop-metrics.properties, hadoop-metrics2.properties
>
>
> The property of dmax in ganglia is a configurable time to rotate metrics.
> Therefore, no more value of the metric will be emit to the gmond, after
> 'dmax' seconds, then gmond will destroy the metric in memory. In Hadoop
> metrics framework, the default value of 'dmax' is 0. It means the gmond will
> never destroy the metric although the metric is disappeared. The gmetad
> daemon also does not delete the rrdtool file forever.
> We need to add a method to configure the default value of dmax for all
> metrics in hadoop.properties.
--
This message was sent by Atlassian JIRA
(v6.1.4#6159)
