You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/09/28 09:23:31 UTC
[GitHub] [hadoop] ZanderXu opened a new pull request, #4945: HDFS-16785. DataNode hold BP write lock to scan disk
ZanderXu opened a new pull request, #4945:
URL: https://github.com/apache/hadoop/pull/4945
### Description of PR
When patching the fine-grained locking of datanode, I found that `addVolume` will hold the write block of the BP lock to scan the new volume to get the blocks. If we try to add one full volume that was fixed offline before, i will hold the write lock for a long time.
The related code as bellows:
```
for (final NamespaceInfo nsInfo : nsInfos) {
String bpid = nsInfo.getBlockPoolID();
try (AutoCloseDataSetLock l = lockManager.writeLock(LockLevel.BLOCK_POOl, bpid)) {
fsVolume.addBlockPool(bpid, this.conf, this.timer);
fsVolume.getVolumeMap(bpid, tempVolumeMap, ramDiskReplicaTracker);
} catch (IOException e) {
LOG.warn("Caught exception when adding " + fsVolume +
". Will throw later.", e);
exceptions.add(e);
}
}
```
And I noticed that this lock is added by [HDFS-15382](https://issues.apache.org/jira/browse/HDFS-15382), means that this logic was not with lock before.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] MingXiangLi commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
MingXiangLi commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1262159782
LGTM @Hexiaoqiao any good suggestion?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] Hexiaoqiao commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
Hexiaoqiao commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1298322152
LGTM. +1 from my side. sorry for the late response.
cc @MingXiangLi any more comments here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] Hexiaoqiao commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
Hexiaoqiao commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1298331657
addendum: 1. almost none cases to trigger more than one thread add volume; 2. even in the worst case, one volume instance will be failed to add to volumeMap when active volume. In one word, I think it is acceptable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ZanderXu commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
ZanderXu commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1311175702
The failed UT `hadoop.hdfs.server.namenode.ha.TestHAStateTransitions` doesn't relate to this PR and it works well locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] Hexiaoqiao commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
Hexiaoqiao commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1309734208
re-trigger jenkins and let's wait what it will say.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] Hexiaoqiao commented on pull request #4945: HDFS-16785. Avoid to hold write lock to improve performance when add volume.
Posted by GitBox <gi...@apache.org>.
Hexiaoqiao commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1312738853
Committed to trunk. Thanks @ZanderXu for your works. And@MingXiangLi @tomscut for your reviews.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1261207290
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 52s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 1s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 1s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 0s | | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 41m 52s | | trunk passed |
| +1 :green_heart: | compile | 1m 42s | | trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 |
| +1 :green_heart: | compile | 1m 30s | | trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | checkstyle | 1m 17s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 36s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 18s | | trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 |
| +1 :green_heart: | javadoc | 1m 38s | | trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 42s | | trunk passed |
| +1 :green_heart: | shadedclient | 29m 12s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 44s | | the patch passed |
| +1 :green_heart: | compile | 1m 41s | | the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 |
| +1 :green_heart: | javac | 1m 41s | | the patch passed |
| +1 :green_heart: | compile | 1m 21s | | the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | javac | 1m 21s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 1m 0s | | the patch passed |
| +1 :green_heart: | mvnsite | 1m 37s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 59s | | the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 |
| +1 :green_heart: | javadoc | 1m 32s | | the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 3m 36s | | the patch passed |
| +1 :green_heart: | shadedclient | 26m 24s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| -1 :x: | unit | 337m 53s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4945/1/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. |
| +1 :green_heart: | asflicense | 0m 55s | | The patch does not generate ASF License warnings. |
| | | 461m 13s | | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hdfs.server.mover.TestMover |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4945/1/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/4945 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux bffb97a6b6f7 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 61a5d265943a4682c93a3d48f23eba8c7d44cf37 |
| Default Java | Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4945/1/testReport/ |
| Max. process+thread count | 2330 (vs. ulimit of 5500) |
| modules | C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4945/1/console |
| versions | git=2.25.1 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] Hexiaoqiao commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
Hexiaoqiao commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1261758130
> 1、When fsVolume.getVolumeMap() scan the bock from disk to add block metadata，it may add new block metadata when another thread add block.
Great point. +1 from my side. So volume level lock will be more proper?
> 2、How we deal conflict when remove BlockPool operating occur at same time.
Not sure if this is block issue. This improvement seems not involve global meta management, only prepare phase here? So any conflict here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] hadoop-yetus commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
hadoop-yetus commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1310202912
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 46m 0s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| -1 :x: | test4tests | 0m 0s | | The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. |
|||| _ trunk Compile Tests _ |
| +1 :green_heart: | mvninstall | 43m 8s | | trunk passed |
| +1 :green_heart: | compile | 1m 30s | | trunk passed |
| +1 :green_heart: | checkstyle | 1m 17s | | trunk passed |
| +1 :green_heart: | mvnsite | 1m 37s | | trunk passed |
| +1 :green_heart: | javadoc | 1m 45s | | trunk passed |
| +1 :green_heart: | spotbugs | 3m 49s | | trunk passed |
| +1 :green_heart: | shadedclient | 26m 7s | | branch has no errors when building and testing our client artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 1m 23s | | the patch passed |
| +1 :green_heart: | compile | 1m 18s | | the patch passed |
| +1 :green_heart: | javac | 1m 18s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 1m 1s | | the patch passed |
| +1 :green_heart: | mvnsite | 1m 26s | | the patch passed |
| +1 :green_heart: | javadoc | 1m 28s | | the patch passed |
| +1 :green_heart: | spotbugs | 3m 32s | | the patch passed |
| +1 :green_heart: | shadedclient | 25m 31s | | patch has no errors when building and testing our client artifacts. |
|||| _ Other Tests _ |
| -1 :x: | unit | 347m 49s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4945/2/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. |
| +1 :green_heart: | asflicense | 1m 16s | | The patch does not generate ASF License warnings. |
| | | 506m 33s | | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | hadoop.hdfs.server.namenode.ha.TestHAStateTransitions |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4945/2/artifact/out/Dockerfile |
| GITHUB PR | https://github.com/apache/hadoop/pull/4945 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux 35e578afdaa6 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / 61a5d265943a4682c93a3d48f23eba8c7d44cf37 |
| Default Java | Red Hat, Inc.-1.8.0_352-b08 |
| Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4945/2/testReport/ |
| Max. process+thread count | 2237 (vs. ulimit of 5500) |
| modules | C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs |
| Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4945/2/console |
| versions | git=2.9.5 maven=3.6.3 spotbugs=4.2.2 |
| Powered by | Apache Yetus 0.14.0 https://yetus.apache.org |
This message was automatically generated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] Hexiaoqiao commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
Hexiaoqiao commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1273259888
I totally agree that we should not hold lock when ever IO operation, especially scan the whole disk, it will be one terrible disaster even at refresh volume. Of course it does not include during restart DataNode instance.
Back to this case. I think the point is that we should only hold block pool lock (maybe write lock here) when get/set `BlockPoolSlice` rather than one coarse grain lock.
So should we split the following segment and only hold lock for `BlockPoolSlice`. And leave other logic without any level locks. I think it could be acceptable if meet any conflicts or other exceptions about only one volume which is being added.
```
try (AutoCloseDataSetLock l = lockManager.writeLock(LockLevel.BLOCK_POOl, bpid)) {
fsVolume.addBlockPool(bpid, this.conf, this.timer);
fsVolume.getVolumeMap(bpid, tempVolumeMap, ramDiskReplicaTracker);
}
```
WDYT? cc @MingXiangLi @ZanderXu
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] tomscut commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
tomscut commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1295157989
> ```
> final FsVolumeImpl fsVolume =
> createFsVolume(sd.getStorageUuid(), sd, location);
> // no need to add lock
> final ReplicaMap tempVolumeMap = new ReplicaMap();
> ArrayList<IOException> exceptions = Lists.newArrayList();
>
> for (final NamespaceInfo nsInfo : nsInfos) {
> String bpid = nsInfo.getBlockPoolID();
> try (AutoCloseDataSetLock l = lockManager.writeLock(LockLevel.BLOCK_POOl, bpid)) {
> fsVolume.addBlockPool(bpid, this.conf, this.timer);
> fsVolume.getVolumeMap(bpid, tempVolumeMap, ramDiskReplicaTracker);
> } catch (IOException e) {
> LOG.warn("Caught exception when adding " + fsVolume +
> ". Will throw later.", e);
> exceptions.add(e);
> }
> }
> ```
>
> The `fsVolume` here is a local temporary variable and still not be added into the `volumes`, and add/remove bp operations just use the volume in `volumes`, so there is no conflicts. So here doesn't need the lock for `BlockPoolSlice`.
>
> @Hexiaoqiao Sir, can check it again?
I agree with @ZanderXu here. +1 from my side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ZanderXu commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
ZanderXu commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1261662338
@MingXiangLi @Hexiaoqiao Sir, can you help me review this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ZanderXu commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
ZanderXu commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1262143802
```
try (AutoCloseDataSetLock l = lockManager.readLock(LockLevel.VOLUME, bpid, fsVolume.getStorageID())) {
fsVolume.addBlockPool(bpid, this.conf, this.timer);
fsVolume.getVolumeMap(bpid, tempVolumeMap, ramDiskReplicaTracker);
} catch (IOException e) {
LOG.warn("Caught exception when adding " + fsVolume +
". Will throw later.", e);
exceptions.add(e);
}
```
Changing the code as above? Emm.. Holding the BP read lock for a long time will have a great impact on the operations that need to acquire the BP write lock, such as: invalidate, recoverAppend, createTemporary.
The current logic uses IOException to avoid the conflict case, I think it's ok. And there is no lock before HDFS-15382, means it's ok. If we can find one conflict case, we can use IOException to fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ZanderXu commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
ZanderXu commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1273427206
```
final FsVolumeImpl fsVolume =
createFsVolume(sd.getStorageUuid(), sd, location);
// no need to add lock
final ReplicaMap tempVolumeMap = new ReplicaMap();
ArrayList<IOException> exceptions = Lists.newArrayList();
for (final NamespaceInfo nsInfo : nsInfos) {
String bpid = nsInfo.getBlockPoolID();
try (AutoCloseDataSetLock l = lockManager.writeLock(LockLevel.BLOCK_POOl, bpid)) {
fsVolume.addBlockPool(bpid, this.conf, this.timer);
fsVolume.getVolumeMap(bpid, tempVolumeMap, ramDiskReplicaTracker);
} catch (IOException e) {
LOG.warn("Caught exception when adding " + fsVolume +
". Will throw later.", e);
exceptions.add(e);
}
}
```
The `fsVolume` here is a local temporary variable and still not be added into the `volumes`, and add/remove bp operations just use the volume in `volumes`, so there is no conflicts. So here doesn't need the lock for `BlockPoolSlice`.
@Hexiaoqiao Sir, can check it again?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] MingXiangLi commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
MingXiangLi commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1261780786
> Great point. +1 from my side. So volume level lock will be more proper?
volume level is ok, It will hold the block pool read lock, to avoid case2 conflict occur。
> Not sure if this is block issue. This improvement seems not involve global meta management, only prepare phase here? So any conflict here?
addVolume() may invoke by refresh Configuration.It may conflict by FsDatasetImpl#shutdownBlockPool.There are two stages in FsDatasetImpl#shutdownBlockPool.For example conflict will occur like this.
`
FsDatasetImpl#shutdownBlockPool*volumeMap.cleanUpBlockPool(bpid);
FsDatasetImpl#addVolume*fsVolume.getVolumeMap(bpid, tempVolumeMap, ramDiskReplicaTracker)
FsDatasetImpl#shutdownBlockPool*volumes.removeBlockPool(bpid, blocksPerVolume);
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ZanderXu commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
ZanderXu commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1294516173
@Hexiaoqiao @tomscut @haiyang1987 @MingXiangLi Sir, can you help me review this PR?
It will block the DataNode for a long time when we dynamically add one disk with many blocks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] Hexiaoqiao commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
Hexiaoqiao commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1298321977
LGTM. +1 from my side. sorry for the late response.
cc @MingXiangLi any more comments here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] MingXiangLi commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
MingXiangLi commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1261752537
I think it's necessary to hold the pool lock.Or make sure the following scenarios not have problem without hold block pool lock for example.
1、When fsVolume.getVolumeMap() scan the bock from disk to add block metadata，it may add new block metadata when another thread add block.
2、How we deal conflict when remove BlockPool operating occur at same time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ZanderXu commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
ZanderXu commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1263411206
@MingXiangLi Sir, thanks for your review.
> Case2: It's wrong, but not caused by this lock.
About this case, I will create one new PR to fix it.
> And can avoid conflict cause by volume/block pool remove or add.
We can add one synchronized lock to `addBlockPool` and `shutdownBlockPool` as before to avoid the conflict caused by volume/blockPool remove or add. And this synchronized just lock volume/blockPool remove or add, so it will not block other operations, such as read or write from client. If ok, I will fix it in a new ticket with Case2 together.
@Hexiaoqiao looking forward your good ideas.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] Hexiaoqiao commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
Hexiaoqiao commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1261825293
> addVolume() may invoke by refresh Configuration.It may conflict by FsDatasetImpl#shutdownBlockPool.There are two stages in FsDatasetImpl#shutdownBlockPool.For example conflict will occur like this.
>
> 1.FsDatasetImpl#shutdownBlockPool*volumeMap.cleanUpBlockPool
> 2.FsDatasetImpl#addVolume*fsVolume.getVolumeMap
> 3.FsDatasetImpl#shutdownBlockPool*volumes.removeBlockPool
Got it. What I mean above is that `fsVolume.getVolumeMap(bpid, tempVolumeMap, ramDiskReplicaTracker);` is absolutely local variable operation, so is it necessary to hold lock here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] ZanderXu commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
ZanderXu commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1261833655
@MingXiangLi @Hexiaoqiao Sir, thanks for the warm discussions.
> 1、When fsVolume.getVolumeMap() scan the bock from disk to add block metadata，it may add new block metadata when another thread add block.
This volume is still not added into the FsVolumeList here, means another thread can not add new block into this volume? So this case not exists?
> 2、How we deal conflict when remove BlockPool operating occur at same time.
RemoveBlockPool just remove the blocks in the memory replicasMap, will not delete blocks on the disk. So remove block pool operation will not affect scanning disk. Let's see some stages:
Case 1: It's ok.
1. fsVolume.addBlockPool();
2. volumeMap.cleanUpBlockPool(bpid);
3. volumes.removeBlockPool(bpid, blocksPerVolume); Here will remove the blockPoolSlice
4. fsVolume.getVolumeMap(); here will throw IOException, because the blockPoolSlice is null.
Case2:
1. addVolume get NamespaceInfo
2. volumeMap.cleanUpBlockPool(bpid);
3. volumes.removeBlockPool(bpid, blocksPerVolume); Here will remove the blockPoolSlice
4. fsVolume.addBlockPool();
5. fsVolume.getVolumeMap(); here will be ok
6. activateVolume(); here will add the removed bpid into the replicamap again. Maybe is not expected. But it's another problem, should be fixed by other PR.
From my point, scanning the disk with a lock for a long time is not allowed. If there are some conflict here, we can find them and fix by other solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] MingXiangLi commented on pull request #4945: HDFS-16785. DataNode hold BP write lock to scan disk
Posted by GitBox <gi...@apache.org>.
MingXiangLi commented on PR #4945:
URL: https://github.com/apache/hadoop/pull/4945#issuecomment-1262013656
> fsVolume.getVolumeMap(); here will throw IOException, because the blockPoolSlice is null.
I think we can change this lock to read lock.This not influences most of read/write operate.And can avoid conflict cause by volume/block pool remove or add.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org
[GitHub] [hadoop] Hexiaoqiao merged pull request #4945: HDFS-16785. Avoid to hold write lock to improve performance when add volume.
Posted by GitBox <gi...@apache.org>.
Hexiaoqiao merged PR #4945:
URL: https://github.com/apache/hadoop/pull/4945
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org