[
https://issues.apache.org/jira/browse/HADOOP-13660?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15605622#comment-15605622
]
Sean Mackrory edited comment on HADOOP-13660 at 10/25/16 3:34 PM:
------------------------------------------------------------------
{quote}IOException is imported but not used in MetricsConfig.java{quote}
Fixed. Also, we're able to use a helper class that makes the new code in this
class a little more concise (fresh patch attached).
{quote}Depending on exception message is generally risky. Would it be possible
to add a regression test? Or is it already covered by existing unit
tests?{quote}
This is already covered by existing unit tests: TestMetricsConfig#testLoadFirst
and TestMetricsConfig#testMissingFiles will both fail if the code doesn't
gracefully detect that one of the files it attempts to load are missing and
proceeds to try elsewhere.
{quote}Want to call this out so we can study it further.{quote}
You mean like a release note? Can definitely add one. I believe the concern is
very minor here, though. I've compared the logic for reading in the
configuration, and the code is functionally equivalent (it's actually mostly
identical). The documented definition of how things get escaped in a list also
remains the same. Where things differ is exactly how things get escaped when
writing back out the configuration, which appears to mainly be done for logging
/ debugging. Even then, it should only be different for some edge cases that as
far as I can tell should come up exceptionally rarely (perhaps never) in the
Metrics2 use case (values that end with backslashes, etc.) Tough to be sure
I've looked at all the use cases exhaustively, though.
was (Author: mackrorysd):
{quote}IOException is imported but not used in MetricsConfig.java{quote}
Fixed. Also, we're able to use a helper class that makes the new code in this
class a little more concise.
{quote}Depending on exception message is generally risky. Would it be possible
to add a regression test? Or is it already covered by existing unit
tests?{quote}
This is already covered by existing unit tests: TestMetricsConfig#testLoadFirst
and TestMetricsConfig#testMissingFiles will both fail if the code doesn't
gracefully detect that one of the files it attempts to load are missing and
proceeds to try elsewhere.
{quote}Want to call this out so we can study it further.{quote}
You mean like a release note? Can definitely add one. I believe the concern is
very minor here, though. I've compared the logic for reading in the
configuration, and the code is functionally equivalent (it's actually mostly
identical). The documented definition of how things get escaped in a list also
remains the same. Where things differ is exactly how things get escaped when
writing back out the configuration, which appears to mainly be done for logging
/ debugging. Even then, it should only be different for some edge cases that as
far as I can tell should come up exceptionally rarely (perhaps never) in the
Metrics2 use case (values that end with backslashes, etc.) Tough to be sure
I've looked at all the use cases exhaustively, though.
> Upgrade commons-configuration version
> -------------------------------------
>
> Key: HADOOP-13660
> URL: https://issues.apache.org/jira/browse/HADOOP-13660
> Project: Hadoop Common
> Issue Type: Improvement
> Components: build
> Affects Versions: 3.0.0-alpha2
> Reporter: Sean Mackrory
> Assignee: Sean Mackrory
> Attachments: HADOOP-13660-configuration2.001.patch,
> HADOOP-13660.001.patch, HADOOP-13660.002.patch, HADOOP-13660.003.patch,
> HADOOP-13660.004.patch
>
>
> We're currently pulling in version 1.6 - I think we should upgrade to the
> latest 1.10.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
