[
https://issues.apache.org/jira/browse/HADOOP-9252?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13569977#comment-13569977
]
Chris Nauroth commented on HADOOP-9252:
---------------------------------------
The changes look great. I applied the patch and ran the new tests and saw them
pass. I think there is one more change we need to make: force locale to
English on the {{String#format}} calls. The old code always used English for
the locale:
{code}
- private static final DecimalFormat decimalFormat;
- static {
- NumberFormat numberFormat =
NumberFormat.getNumberInstance(Locale.ENGLISH);
- decimalFormat = (DecimalFormat) numberFormat;
- decimalFormat.applyPattern("#.##");
- }
-
{code}
The current version of the patch uses the platform default locale. Here is an
example that demonstrates what happens when running on a JVM where the default
locale has been set to French, which uses comma instead of period for the
decimal separator.
{code}
scala> Locale.setDefault(Locale.ENGLISH)
scala> println(StringUtils.formatPercent(0.1234, 2))
12.34%
scala> println(StringUtils.humanReadableInt(123456))
warning: there were 1 deprecation warnings; re-run with -deprecation for details
120.6 K
scala> println(StringUtils.limitDecimalTo2(876.5432))
warning: there were 1 deprecation warnings; re-run with -deprecation for details
876.54
scala> println(StringUtils.TraditionalBinaryPrefix.long2String(3L << 9, null,
2))
1.50 K
scala> Locale.setDefault(Locale.FRENCH)
scala> println(StringUtils.formatPercent(0.1234, 2))
12,34%
scala> println(StringUtils.humanReadableInt(123456))
warning: there were 1 deprecation warnings; re-run with -deprecation for details
120,6 K
scala> println(StringUtils.limitDecimalTo2(876.5432))
warning: there were 1 deprecation warnings; re-run with -deprecation for details
876,54
scala> println(StringUtils.TraditionalBinaryPrefix.long2String(3L << 9, null,
2))
1,50 K
{code}
I'm guessing that we want to stick with English, because the surrounding pieces
that use these methods, like the NameNode UI, aren't currently localizable and
are basically hard-coded to US-style English. Also, the tests could fail for
developers using a system with a different default locale. The
{{String#format}} calls would change from this:
{code}
+ return String.format("%." + decimalPlaces + "f%%", fraction*100);
+ b.append(String.format("%." + decimalPlaces + "f", d));
+ return String.format("%.2f", d);
{code}
to this:
{code}
+ return String.format(Locale.ENGLISH, "%." + decimalPlaces + "f%%",
fraction*100);
+ b.append(String.format(Locale.ENGLISH, "%." + decimalPlaces + "f",
d));
+ return String.format(Locale.ENGLISH, "%.2f", d);
{code}
Thanks!
> StringUtils.limitDecimalTo2(..) is unnecessarily synchronized
> -------------------------------------------------------------
>
> Key: HADOOP-9252
> URL: https://issues.apache.org/jira/browse/HADOOP-9252
> Project: Hadoop Common
> Issue Type: Improvement
> Components: util
> Reporter: Tsz Wo (Nicholas), SZE
> Assignee: Tsz Wo (Nicholas), SZE
> Priority: Minor
> Attachments: c9252_20130127.patch, c9252_20130128.patch,
> c9252_20130203.patch
>
>
> limitDecimalTo2(double) currently uses decimalFormat, which is a static
> field, so that it is synchronized. Synchronization is unnecessary since it
> can simply uses String.format(..).
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
