[
https://issues.apache.org/jira/browse/HADOOP-11361?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15374164#comment-15374164
]
Tsuyoshi Ozawa edited comment on HADOOP-11361 at 7/13/16 1:58 AM:
------------------------------------------------------------------
Thanks [~vinayrpet] and [~yzhangal] for your reviews. My comments are as
follows:
1. . This is related to Youngjun's comment about null check, I perfer to remove
the variable {{MetricsCollectorImpl builder}} and to use local variable
{{List<MetricsRecordImpl> lastRecs}} for simplicity:
{code}
List<MetricsRecordImpl> lastRecs = null;
if (getAllMetrics) {
lastRecs = getMetrics(new MetricsCollectorImpl());
}
synchronized(this) {
if (lastRecs != null) {
updateAttrCache(lastRecs);
if (getAllMetrics) {
updateInfoCache(lastRecs);
}
}
jmxCacheTS = Time.now();
lastRecsCleared = true;
}
{code}
In this case, we also need to update {{getMetrics(MetricsCollectorImpl
builder)}} to return {{List<MetricsRecordImpl>}} instead of
{{Iterable<MetricsRecordImpl>}}.
2. This is minor nits comment, but I think we should add null-check assertions
against lastRecs in following methods: {{private void
updateInfoCache(List<MetricsRecordImpl> lastRecs)}}, {{private int
updateAttrCache(List<MetricsRecordImpl> lastRecs)}}. It increases readability
and simplicity. On trunk, we can use {{Nonnull}}
annotation(https://blogs.oracle.com/java-platform-group/entry/java_8_s_new_type),
but it's been introduced since jdk8. Assert.checkNotNull provided in Guava is
also enough.
was (Author: ozawa):
Thanks [~vinayrpet] and [~yzhangal] for your reviews. My comments are as
follows:
1. . This is related to Youngjun's comment about null check, I perfer to remove
the variable {{MetricsCollectorImpl builder}} and to use local variable
{{List<MetricsRecordImpl> lastRecs}} for simplicity:
{code}
List<MetricsRecordImpl> lastRecs = null;
if (getAllMetrics) {
lastRecs = getMetrics(new MetricsCollectorImpl());
}
synchronized(this) {
if (lastRecs != null) {
updateAttrCache(lastRecs);
if (getAllMetrics) {
updateInfoCache(lastRecs);
}
}
jmxCacheTS = Time.now();
lastRecsCleared = true;
}
{code}
In this case, we also need to update {{getMetrics(MetricsCollectorImpl
builder)}} to return {{List<MetricsRecordImpl>}} instead of
{{Iterable<MetricsRecordImpl>}}.
2. This is minor nits comment, but I think we should add null-check assertions
against lastRecs here: {{private void updateInfoCache(List<MetricsRecordImpl>
lastRecs)}}, {{private int updateAttrCache(List<MetricsRecordImpl> lastRecs)}}.
It increases readability and simplicity. On trunk, we can use {{Nonnull}}
annotation(https://blogs.oracle.com/java-platform-group/entry/java_8_s_new_type),
but it's been introduced since jdk8. Assert.checkNotNull provided in Guava is
also enough.
> Fix a race condition in MetricsSourceAdapter.updateJmxCache
> -----------------------------------------------------------
>
> Key: HADOOP-11361
> URL: https://issues.apache.org/jira/browse/HADOOP-11361
> Project: Hadoop Common
> Issue Type: Bug
> Affects Versions: 2.4.1, 2.5.1, 2.6.0
> Reporter: Brahma Reddy Battula
> Assignee: Brahma Reddy Battula
> Attachments: HADOOP-111361-003.patch, HADOOP-11361-002.patch,
> HADOOP-11361-004.patch, HADOOP-11361-005.patch, HADOOP-11361-005.patch,
> HADOOP-11361.patch, HDFS-7487.patch
>
>
> {noformat}
> Caused by: java.lang.NullPointerException
> at
> org.apache.hadoop.metrics2.impl.MetricsSourceAdapter.updateAttrCache(MetricsSourceAdapter.java:247)
> at
> org.apache.hadoop.metrics2.impl.MetricsSourceAdapter.updateJmxCache(MetricsSourceAdapter.java:177)
> at
> org.apache.hadoop.metrics2.impl.MetricsSourceAdapter.getAttribute(MetricsSourceAdapter.java:102)
> at
> com.sun.jmx.interceptor.DefaultMBeanServerInterceptor.getAttribute(DefaultMBeanServerInterceptor.java:647)
> {noformat}
--
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
