Thanks ! okay that helps to clarify things.
Okay, so the value in referencing the static comment is that it is
commented in the DFS_KEYS file, and declared as 0. Having the ==0 in code
defines this default behaviour implicitly, so a change to the code would
make that code inconsistent with the comment in the DFS Keys file. Having
run into some tricky configuration changes in the past it concerns me a
little... but...
The more generic question is wether or not there is enforcement of naming
conventions or commenting for special values in numeric configuration
parameters: I'm interpretting your answer as "No"...?
Thats fine.. but just clarifying :) ?
On Fri, Apr 19, 2013 at 2:23 PM, Aaron T. Myers <a...@cloudera.com> wrote:
> Hi Jay,
>
> On Sat, Apr 20, 2013 at 1:10 AM, Jay Vyas <jayunit...@gmail.com> wrote:
>
> > I recently looked into the HDFS source tree to determine idioms with
> > respect to a hairy debate about the threshold between what is and is not
> a
> > magic number, and found that : And it appears that the number zero is
> NOT
> > considered magic - at least not in the HDFS source code.
> >
>
> It's certainly not magic in the Configuration class interpretation of it,
> though I think if you surveyed the full source code you'd find that there
> won't be much consistency with regard to ad hoc checks in the code for
> certain values, like you've identified below.
>
>
> >
> > I found that that DFSConfigKeys.java defines DEFAULT values of zeros for
> > some fields, and those defaults result in non-quantitative interpretation
> > of the field.
> >
> > For example:
> > dfs.image.transfer.bandwidthPerSec
> >
> > Is commented like so:
> > public static final long DFS_IMAGE_TRANSFER_RATE_DEFAULT = 0; //no
> > throttling
> >
> > However in the implementation of these defaults, "magic" zeros are used
> > without commenting:
> > if (transferBandwidth > 0) {
> > throttler = new DataTransferThrottler(transferBandwidth);
> > }
> >
> > --------
> >
> > Seems like the 0 above would be better replaced with
> > DFS_IMAGE_TRANSFER_RATE_DEFAULT since the "no throttling" behaviour is
> > defined with the constant in the DFSConfigKeys file, and not defined in
> the
> >
> >
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/GetImageServlet.java.
> >
>
> I don't agree with this. What if we later changed the default to something
> greater than 0 in DFSConfigKeys? If the code were comparing against the
> value DFS_IMAGE_TRANSFER_RATE_DEFAULT, the check in the code would then be
> wrong. The only value for that config that should denote "no throttling" is
> 0, regardless of what the default is, so the explicit comparison against 0
> makes sense to me.
>
>
> >
> >
> > --------
> >
> > Trying to get a feel for if there are conventions to enforce in some
> code
> > reviews for our hadoop dependent configuration code. We'd like to follow
> > hadoopy idioms if possible..
> >
>
> I'd say the main conventions you should concern yourself with for this
> purpose is config setting naming, e.g. use consistent prefixes within your
> own code, use lower case separated by dots.and-dashes, etc.
>
>
> >
> >
> > --
> > Jay Vyas
> > http://jayunit100.blogspot.com
> >
>
--
Jay Vyas
http://jayunit100.blogspot.com
