[
https://issues.apache.org/jira/browse/HDFS-11267?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15783620#comment-15783620
]
Manoj Govindassamy commented on HDFS-11267:
-------------------------------------------
Thanks for the review [~eddyxu].
{noformat}
index
1f03fc27cd7c6ea9a0f65d8f0417d581172796ba..2cca60e25bc7cc343bb492bbf464fbc73ba21c23
100644
---
a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
+++
b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
@@ -123,7 +123,7 @@
public boolean isOfType(StorageDirType type);
}
- protected List<StorageDirectory> storageDirs = new
ArrayList<StorageDirectory>();
+ private List<StorageDirectory> storageDirs;
..
..
+
+ public List<StorageDirectory> getStorageDirs() {
+ return storageDirs;
+ }
+
{noformat}
# Protected vs Private:
v01 patch already converted the "protected List" to a "private List",
initialized it only in the constructor and added getStorageDirs().
Yes, the second template parameter in the constructor is not needed. Removing
it.
# Thread Safety:
The idea is to use {{CopyonWriteArrayList}} in the base Storage class for
storageDirs. The proposed patch is missing the CopyOnWriteArrayList as I
assumed HDFS-11251 will bring in that fix. But, thinking more about it, its
better to make the fix for this issue a self contained one and so will amend
the patch with storageDirs initialized with CopyOnWriteArrayList.
After storageDirs is made a {{CopyOnWriteArrayList}}, the iterator will use a
snapshot of the underlying list and the iterator will never throw a
{{ConcurrentModificationException}}. There can be parallel addition and removal
of storageDirs and still other threads can iterate over the storageDirs without
facing any issues (because, they are iterating on the point in time copy of
List).
So after CopyOnWriteArrayList change, it will be thread safe.
# NNStorage concurrency
Since we are making the base Storage class storageDirs a CopyOnWriteArrayList,
there will be no weakening of concurrency protection for NNStorage.
> 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
