[jira] [Commented] (HDFS-11267) Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in Storage
[
https://issues.apache.org/jira/browse/HDFS-11267?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15784978#comment-15784978
]
Hadoop QA commented on HDFS-11267:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
0s{color} | {color:blue} Docker mode activated. {color} |
| {color:red}-1{color} | {color:red} patch {color} | {color:red} 0m 4s{color}
| {color:red} HDFS-11267 does not apply to trunk. Rebase required? Wrong
Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. {color} |
\\
\\
|| Subsystem || Report/Notes ||
| JIRA Issue | HDFS-11267 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12845033/HDFS-11267.03.patch |
| Console output |
https://builds.apache.org/job/PreCommit-HDFS-Build/17984/console |
| Powered by | Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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, HDFS-11267.02.patch,
> HDFS-11267.03.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
[jira] [Commented] (HDFS-11267) Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in Storage
[
https://issues.apache.org/jira/browse/HDFS-11267?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15784957#comment-15784957
]
Hudson commented on HDFS-11267:
---
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #11055 (See
[https://builds.apache.org/job/Hadoop-trunk-Commit/11055/])
HDFS-11267. Avoid redefinition of storageDirs in NNStorage and cleanup (lei:
rev a4f66655ec22ca8c960f971f2b0cdafbd3430ad7)
* (edit)
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/datanode/TestBlockPoolSliceStorage.java
* (edit)
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/BlockPoolSliceStorage.java
* (edit)
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataStorage.java
* (edit)
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/common/StorageAdapter.java
* (edit)
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/common/Storage.java
* (edit)
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
> 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, HDFS-11267.02.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
[jira] [Commented] (HDFS-11267) Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in Storage
[
https://issues.apache.org/jira/browse/HDFS-11267?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15784927#comment-15784927
]
Hadoop QA commented on HDFS-11267:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
0s{color} | {color:blue} Docker mode activated. {color} |
| {color:red}-1{color} | {color:red} patch {color} | {color:red} 0m 5s{color}
| {color:red} HDFS-11267 does not apply to trunk. Rebase required? Wrong
Branch? See https://wiki.apache.org/hadoop/HowToContribute for help. {color} |
\\
\\
|| Subsystem || Report/Notes ||
| JIRA Issue | HDFS-11267 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12844975/HDFS-11267.02.patch |
| Console output |
https://builds.apache.org/job/PreCommit-HDFS-Build/17983/console |
| Powered by | Apache Yetus 0.5.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
> 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, HDFS-11267.02.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
[jira] [Commented] (HDFS-11267) Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in Storage
[
https://issues.apache.org/jira/browse/HDFS-11267?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15784306#comment-15784306
]
Manoj Govindassamy commented on HDFS-11267:
---
Thanks for the review [~linyiqun].
> 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, HDFS-11267.02.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
[jira] [Commented] (HDFS-11267) Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in Storage
[
https://issues.apache.org/jira/browse/HDFS-11267?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15784243#comment-15784243
]
Yiqun Lin commented on HDFS-11267:
--
The latest patch LGTM, +1. Thanks [~manojg] for the follow-up work of
HDFS-11251.
> 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, HDFS-11267.02.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
[jira] [Commented] (HDFS-11267) Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in Storage
[
https://issues.apache.org/jira/browse/HDFS-11267?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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 storageDirs = new
ArrayList();
+ private List storageDirs;
..
..
+
+ public List 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
[jira] [Commented] (HDFS-11267) Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in Storage
[
https://issues.apache.org/jira/browse/HDFS-11267?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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 storageDirs = new
ArrayList();
{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 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
[jira] [Commented] (HDFS-11267) Avoid redefinition of storageDirs in NNStorage and cleanup its accessors in Storage
[
https://issues.apache.org/jira/browse/HDFS-11267?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15781739#comment-15781739
]
Manoj Govindassamy commented on HDFS-11267:
---
The fix for HDFS-11251 (ConcurrentModificationException during
DataNode#refreshVolumes) making the {{Storage#storageDirs}} a
{{CopyOnWriteArrayList}} is needed for this bug. On top of HDFS-11251 fix, we
need more clean up patches.
> 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
>
> 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
