[
https://issues.apache.org/jira/browse/HDFS-11267?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15781957#comment-15781957
]
Lei (Eddy) Xu commented on HDFS-11267:
--------------------------------------
Hi, [~manojg]
Thanks a lot for cleaning the code.
I have a few questions remained:
{code}
protected List<StorageDirectory> storageDirs = new
ArrayList<StorageDirectory>();
{code}
Can we change it to {{private}} and adds the {{public getStorageDirs()}}?
Adding {{new ArrayList<>()}} to constructor makes duplicated code. Btw, as for
JDK 7, the second template parameter can be removed:
{code}
protected List<StorageDirectory> storageDirs = new ArrayList<>();
{code}
* Regarding thread-safe iteration, the proposed patch can protected get/set the
reference of the StorageDir collection. But it might be difficult to protect
iterating the collection.
* Would using {{ArrayList()}} in {{NNStorage}} weaken the concurrency
protection of {{NNStorage}}?
Thanks!
> Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in
> Storage
> -----------------------------------------------------------------------------------
>
> Key: HDFS-11267
> URL: https://issues.apache.org/jira/browse/HDFS-11267
> Project: Hadoop HDFS
> Issue Type: Bug
> Affects Versions: 3.0.0-alpha1
> Reporter: Manoj Govindassamy
> Assignee: Manoj Govindassamy
> Attachments: HDFS-11267.01.patch
>
>
> In the abstract class {{Storage}}, {{storageDirs}} is a protected variable
> and all its derived classes like {{NNStorage}}, {{JNStorage}},
> {{DataStorage}}.. are iterating over this non-thread safe variable without
> any proper locks. Any parallel modification operation like add or remove
> volume can mutate the backing storageDirs list and any iterators on this list
> around the same time can face {{ConcurrentModificationException}}. It would
> be good to make the variable private and restrict the access via getters and
> setters. Any thread safe restriction need to be done can then be placed on
> the parent class only.
> Also, {{NNStorage}} redefines parent class Storage's {{storageDirs}}, making
> it inconsistent with other derived classes. Would be cleaner if {{NNStorage}}
> can avoid re-defining it locally.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
