Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
kokon191 commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2576604154 Failed tests seem to be unrelated to FGL. In particular, hadoop.hdfs.server.blockmanagement.TestBlockTokenWithDFSStriped failure was caused by HDFS-17080 and hadoop.hdfs.server.federation.store.driver.TestStateStoreFileSystem by HDFS-17496 -- 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hadoop-yetus commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2575025799 :broken_heart: **-1 overall** | Vote | Subsystem | Runtime | Logfile | Comment | |::|--:|:|::|:---:| | +0 :ok: | reexec | 0m 42s | | Docker mode activated. | _ Prechecks _ | | +1 :green_heart: | dupname | 0m 1s | | No case conflicting files found. | | +0 :ok: | codespell | 0m 1s | | codespell was not available. | | +0 :ok: | detsecrets | 0m 1s | | detect-secrets was not available. | | +0 :ok: | xmllint | 0m 1s | | xmllint was not available. | | +0 :ok: | markdownlint | 0m 1s | | markdownlint was not available. | | +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. | | +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 49 new or modified test files. | _ trunk Compile Tests _ | | +0 :ok: | mvndep | 9m 18s | | Maven dependency ordering for branch | | +1 :green_heart: | mvninstall | 30m 41s | | trunk passed | | +1 :green_heart: | compile | 16m 43s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | compile | 14m 56s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | checkstyle | 4m 18s | | trunk passed | | +1 :green_heart: | mvnsite | 3m 10s | | trunk passed | | +1 :green_heart: | javadoc | 2m 59s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javadoc | 3m 24s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 5m 47s | | trunk passed | | +1 :green_heart: | shadedclient | 33m 1s | | branch has no errors when building and testing our client artifacts. | | -0 :warning: | patch | 33m 27s | | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. | _ Patch Compile Tests _ | | +0 :ok: | mvndep | 0m 32s | | Maven dependency ordering for patch | | +1 :green_heart: | mvninstall | 1m 55s | | the patch passed | | +1 :green_heart: | compile | 16m 10s | | the patch passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javac | 16m 10s | | the patch passed | | +1 :green_heart: | compile | 15m 16s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | javac | 15m 16s | | the patch passed | | +1 :green_heart: | blanks | 0m 1s | | The patch has no blanks issues. | | +1 :green_heart: | checkstyle | 4m 21s | | root: The patch generated 0 new + 1233 unchanged - 16 fixed = 1233 total (was 1249) | | +1 :green_heart: | mvnsite | 3m 11s | | the patch passed | | +1 :green_heart: | javadoc | 1m 18s | | hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 generated 0 new + 3113 unchanged - 4 fixed = 3113 total (was 3117) | | +1 :green_heart: | javadoc | 0m 53s | | hadoop-hdfs-rbf in the patch passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04. | | +1 :green_heart: | javadoc | 0m 44s | | hadoop-fs2img in the patch passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04. | | +1 :green_heart: | javadoc | 3m 15s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 6m 21s | | the patch passed | | +1 :green_heart: | shadedclient | 33m 51s | | patch has no errors when building and testing our client artifacts. | _ Other Tests _ | | -1 :x: | unit | 230m 47s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/9/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. | | -1 :x: | unit | 30m 59s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs-rbf.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/9/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs-rbf.txt) | hadoop-hdfs-rbf in the patch passed. | | +1 :green_heart: | unit | 0m 59s | | hadoop-fs2img in the patch passed. | | +1 :green_heart: | asflicense | 1m 14s | | The patch does not generate ASF License warnings. | | | | 483m 32s | | | | Reason | Tests | |---:|:--| | Failed junit tests | hadoop.hdfs.server.blockmanagement.TestBlockTokenWithDFSStriped | | | hadoop.hdfs.server.federation.store.driver.TestStateStoreFileSystem | | Subsystem | Report/Notes | |--
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hadoop-yetus commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2566382405 :broken_heart: **-1 overall** | Vote | Subsystem | Runtime | Logfile | Comment | |::|--:|:|::|:---:| | +0 :ok: | reexec | 0m 31s | | Docker mode activated. | _ Prechecks _ | | +1 :green_heart: | dupname | 0m 2s | | No case conflicting files found. | | +0 :ok: | codespell | 0m 0s | | codespell was not available. | | +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. | | +0 :ok: | xmllint | 0m 0s | | xmllint was not available. | | +0 :ok: | markdownlint | 0m 0s | | markdownlint was not available. | | +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. | | +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 49 new or modified test files. | _ trunk Compile Tests _ | | +0 :ok: | mvndep | 7m 24s | | Maven dependency ordering for branch | | +1 :green_heart: | mvninstall | 30m 20s | | trunk passed | | +1 :green_heart: | compile | 16m 31s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | compile | 14m 42s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | checkstyle | 4m 20s | | trunk passed | | +1 :green_heart: | mvnsite | 3m 12s | | trunk passed | | +1 :green_heart: | javadoc | 3m 3s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javadoc | 3m 22s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 5m 44s | | trunk passed | | +1 :green_heart: | shadedclient | 33m 8s | | branch has no errors when building and testing our client artifacts. | | -0 :warning: | patch | 33m 34s | | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. | _ Patch Compile Tests _ | | +0 :ok: | mvndep | 0m 32s | | Maven dependency ordering for patch | | +1 :green_heart: | mvninstall | 1m 56s | | the patch passed | | +1 :green_heart: | compile | 15m 45s | | the patch passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javac | 15m 45s | | the patch passed | | +1 :green_heart: | compile | 15m 13s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | javac | 15m 13s | | the patch passed | | +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. | | +1 :green_heart: | checkstyle | 4m 20s | | root: The patch generated 0 new + 1233 unchanged - 16 fixed = 1233 total (was 1249) | | +1 :green_heart: | mvnsite | 3m 9s | | the patch passed | | +1 :green_heart: | javadoc | 1m 19s | | hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 generated 0 new + 3113 unchanged - 4 fixed = 3113 total (was 3117) | | +1 :green_heart: | javadoc | 0m 54s | | hadoop-hdfs-rbf in the patch passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04. | | +1 :green_heart: | javadoc | 0m 45s | | hadoop-fs2img in the patch passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04. | | +1 :green_heart: | javadoc | 3m 28s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 6m 19s | | the patch passed | | +1 :green_heart: | shadedclient | 33m 30s | | patch has no errors when building and testing our client artifacts. | _ Other Tests _ | | -1 :x: | unit | 224m 24s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/8/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. | | +1 :green_heart: | unit | 30m 22s | | hadoop-hdfs-rbf in the patch passed. | | +1 :green_heart: | unit | 0m 59s | | hadoop-fs2img in the patch passed. | | +1 :green_heart: | asflicense | 1m 13s | | The patch does not generate ASF License warnings. | | | | 473m 10s | | | | Reason | Tests | |---:|:--| | Failed junit tests | hadoop.hdfs.TestDecommissionWithBackoffMonitor | | | hadoop.hdfs.TestDecommission | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/8/artifact/out/Dockerfile | | GITHUB PR | https://github.com/apache/hadoop/pull/6762 | |
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hadoop-yetus commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2564408198 :broken_heart: **-1 overall** | Vote | Subsystem | Runtime | Logfile | Comment | |::|--:|:|::|:---:| | +0 :ok: | reexec | 0m 31s | | Docker mode activated. | _ Prechecks _ | | +1 :green_heart: | dupname | 0m 2s | | No case conflicting files found. | | +0 :ok: | codespell | 0m 0s | | codespell was not available. | | +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. | | +0 :ok: | xmllint | 0m 0s | | xmllint was not available. | | +0 :ok: | markdownlint | 0m 0s | | markdownlint was not available. | | +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. | | +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 49 new or modified test files. | _ trunk Compile Tests _ | | +0 :ok: | mvndep | 6m 1s | | Maven dependency ordering for branch | | +1 :green_heart: | mvninstall | 31m 24s | | trunk passed | | +1 :green_heart: | compile | 17m 7s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | compile | 15m 0s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | checkstyle | 4m 22s | | trunk passed | | +1 :green_heart: | mvnsite | 2m 53s | | trunk passed | | +1 :green_heart: | javadoc | 2m 48s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javadoc | 3m 11s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 5m 49s | | trunk passed | | +1 :green_heart: | shadedclient | 33m 57s | | branch has no errors when building and testing our client artifacts. | | -0 :warning: | patch | 34m 22s | | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. | _ Patch Compile Tests _ | | +0 :ok: | mvndep | 0m 31s | | Maven dependency ordering for patch | | +1 :green_heart: | mvninstall | 1m 54s | | the patch passed | | +1 :green_heart: | compile | 17m 18s | | the patch passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javac | 17m 18s | | the patch passed | | +1 :green_heart: | compile | 15m 6s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | javac | 15m 6s | | the patch passed | | +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. | | +1 :green_heart: | checkstyle | 4m 10s | | root: The patch generated 0 new + 1233 unchanged - 16 fixed = 1233 total (was 1249) | | +1 :green_heart: | mvnsite | 2m 49s | | the patch passed | | -1 :x: | javadoc | 1m 17s | [/results-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/7/artifact/out/results-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04.txt) | hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 generated 2 new + 3115 unchanged - 2 fixed = 3117 total (was 3117) | | +1 :green_heart: | javadoc | 3m 5s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 6m 39s | | the patch passed | | +1 :green_heart: | shadedclient | 39m 11s | | patch has no errors when building and testing our client artifacts. | _ Other Tests _ | | -1 :x: | unit | 225m 4s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/7/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. | | +1 :green_heart: | unit | 30m 19s | | hadoop-hdfs-rbf in the patch passed. | | +1 :green_heart: | unit | 0m 59s | | hadoop-fs2img in the patch passed. | | +1 :green_heart: | asflicense | 1m 14s | | The patch does not generate ASF License warnings. | | | | 480m 18s | | | | Reason | Tests | |---:|:--| | Failed junit tests | hadoop.hdfs.TestDecommissionWithBackoffMonitor | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/7/artifact/out/Dockerfile | | GITHUB PR | https://github.com/apache/hadoop/pull/6762 | | Optional Tests | dupname asflicense com
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
ZanderXu commented on code in PR #6762: URL: https://github.com/apache/hadoop/pull/6762#discussion_r1897862361 ## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/fgl/FineGrainedFSNamesystemLock.java: ## @@ -0,0 +1,300 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hdfs.server.namenode.fgl; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hdfs.server.namenode.FSNamesystemLock; +import org.apache.hadoop.metrics2.lib.MutableRatesWithAggregation; + +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Supplier; + +/** + * Splitting the global FSN lock into FSLock and BMLock. + * FSLock is used to protect directory tree-related operations. + * BMLock is used to protect block-related and dn-related operations. + * The lock order should be: FSLock,BMLock. + */ +public class FineGrainedFSNamesystemLock implements FSNLockManager { + private final FSNamesystemLock fsLock; + private final FSNamesystemLock bmLock; + + public FineGrainedFSNamesystemLock(Configuration conf, MutableRatesWithAggregation aggregation) { +this.fsLock = new FSNamesystemLock(conf, "FS", aggregation); +this.bmLock = new FSNamesystemLock(conf, "BM", aggregation); + } + + @Override + public void readLock(FSNamesystemLockMode lockMode) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.readLock(); + this.bmLock.readLock(); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readLock(); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readLock(); +} + } + + public void readLockInterruptibly(FSNamesystemLockMode lockMode) throws InterruptedException { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.readLockInterruptibly(); + try { +this.bmLock.readLockInterruptibly(); + } catch (InterruptedException e) { +// The held FSLock should be released if the current thread is interrupted +// while acquiring the BMLock. +this.fsLock.readUnlock("BMReadLockInterruptiblyFailed"); +throw e; + } +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readLockInterruptibly(); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readLockInterruptibly(); +} + } + + @Override + public void readUnlock(FSNamesystemLockMode lockMode, String opName) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.readUnlock(opName); + this.fsLock.readUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readUnlock(opName); +} + } + + public void readUnlock(FSNamesystemLockMode lockMode, String opName, + Supplier lockReportInfoSupplier) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.readUnlock(opName, lockReportInfoSupplier); + this.fsLock.readUnlock(opName, lockReportInfoSupplier); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readUnlock(opName, lockReportInfoSupplier); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readUnlock(opName, lockReportInfoSupplier); +} + } + + @Override + public void writeLock(FSNamesystemLockMode lockMode) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.writeLock(); + this.bmLock.writeLock(); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeLock(); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.writeLock(); +} + } + + @Override + public void writeUnlock(FSNamesystemLockMode lockMode, String opName) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.writeUnlock(opName); + this.fsLock.writeUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.write
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
ZanderXu commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2562240142 > I know this PR is phase 1 work: split global lock into fslock and bmlock. We should ensure the lock order: bmlock cannot occur in front of fslock. Can we add some restrictions or check mechanisms to avoid getting a bmlock before fslock? Thanks, @zhengchenyu . This is a good idea. The check mechanism is theoretically feasible (verifying whether the BMLock is already held when acquiring the FSLock). However, due to lock reentry, this approach introduces complexity and negatively impacts the performance. Let's briefly consider some common scenarios: 1. A thread acquires the FS read lock for the first time. 2. A thread acquires the FS write lock for the first time. 3. A thread holding the Global read lock reenters the FS read lock (already holding the BM read lock). 4. A thread holding the Global write lock reenters the FS read lock (already holding the BM write lock). 5. A thread holding the Global write lock reenters the FS write lock (already holding the BM write lock). In scenarios 3, 4, and 5, the BM lock already held is normal when acquiring FS lock. However, in scenarios 1 and 2, the BM lock already held is abnormal when acquiring FS lock. Indeed, lock reentry can also be checked, but I think it’s unnecessary as it’s overly complicated. We can use other methods to detect deadlock issues. -- 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
ZanderXu commented on code in PR #6762: URL: https://github.com/apache/hadoop/pull/6762#discussion_r1897678303 ## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/fgl/FineGrainedFSNamesystemLock.java: ## @@ -0,0 +1,300 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hdfs.server.namenode.fgl; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hdfs.server.namenode.FSNamesystemLock; +import org.apache.hadoop.metrics2.lib.MutableRatesWithAggregation; + +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Supplier; + +/** + * Splitting the global FSN lock into FSLock and BMLock. + * FSLock is used to protect directory tree-related operations. + * BMLock is used to protect block-related and dn-related operations. + * The lock order should be: FSLock,BMLock. + */ +public class FineGrainedFSNamesystemLock implements FSNLockManager { + private final FSNamesystemLock fsLock; + private final FSNamesystemLock bmLock; + + public FineGrainedFSNamesystemLock(Configuration conf, MutableRatesWithAggregation aggregation) { +this.fsLock = new FSNamesystemLock(conf, "FS", aggregation); +this.bmLock = new FSNamesystemLock(conf, "BM", aggregation); + } + + @Override + public void readLock(FSNamesystemLockMode lockMode) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.readLock(); + this.bmLock.readLock(); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readLock(); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readLock(); +} + } + + public void readLockInterruptibly(FSNamesystemLockMode lockMode) throws InterruptedException { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.readLockInterruptibly(); + try { +this.bmLock.readLockInterruptibly(); + } catch (InterruptedException e) { +// The held FSLock should be released if the current thread is interrupted +// while acquiring the BMLock. +this.fsLock.readUnlock("BMReadLockInterruptiblyFailed"); +throw e; + } +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readLockInterruptibly(); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readLockInterruptibly(); +} + } + + @Override + public void readUnlock(FSNamesystemLockMode lockMode, String opName) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.readUnlock(opName); + this.fsLock.readUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readUnlock(opName); +} + } + + public void readUnlock(FSNamesystemLockMode lockMode, String opName, + Supplier lockReportInfoSupplier) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.readUnlock(opName, lockReportInfoSupplier); + this.fsLock.readUnlock(opName, lockReportInfoSupplier); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readUnlock(opName, lockReportInfoSupplier); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readUnlock(opName, lockReportInfoSupplier); +} + } + + @Override + public void writeLock(FSNamesystemLockMode lockMode) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.writeLock(); + this.bmLock.writeLock(); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeLock(); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.writeLock(); +} + } + + @Override + public void writeUnlock(FSNamesystemLockMode lockMode, String opName) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.writeUnlock(opName); + this.fsLock.writeUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.write
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
ZanderXu commented on code in PR #6762: URL: https://github.com/apache/hadoop/pull/6762#discussion_r1897589520 ## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java: ## @@ -2058,7 +2080,7 @@ BatchedListEntries listOpenFiles(long prevId, public BatchedListEntries getFilesBlockingDecom(long prevId, String path) { -assert hasReadLock(); +assert hasReadLock(RwLockMode.FS); Review Comment: Thanks @hfutatzhanghb for your review. Both `dataNode.getLeavingServiceStatus().getOpenFiles()` and `blockManager.getDatanodeManager().getDatanodes())` are already thread-safe, so the BMLock is unnecessary 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hfutatzhanghb commented on code in PR #6762: URL: https://github.com/apache/hadoop/pull/6762#discussion_r1897587322 ## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java: ## @@ -2031,7 +2053,7 @@ BatchedListEntries listOpenFiles(long prevId, BatchedListEntries batchedListEntries; String normalizedPath = new Path(path).toString(); // normalize path. try { - readLock(); + readLock(RwLockMode.FS); Review Comment: Also use GLOBAL mode here, because it invoke getFilesBlockingDecom method. ## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java: ## @@ -2058,7 +2080,7 @@ BatchedListEntries listOpenFiles(long prevId, public BatchedListEntries getFilesBlockingDecom(long prevId, String path) { -assert hasReadLock(); +assert hasReadLock(RwLockMode.FS); Review Comment: I think we should use GLOBAL mode here, because below codes contain iterate `blockManager.getDatanodeManager().getDatanodes()` -- 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hadoop-yetus commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2561959139 :broken_heart: **-1 overall** | Vote | Subsystem | Runtime | Logfile | Comment | |::|--:|:|::|:---:| | +0 :ok: | reexec | 0m 33s | | Docker mode activated. | _ Prechecks _ | | +1 :green_heart: | dupname | 0m 2s | | No case conflicting files found. | | +0 :ok: | codespell | 0m 0s | | codespell was not available. | | +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. | | +0 :ok: | xmllint | 0m 0s | | xmllint was not available. | | +0 :ok: | markdownlint | 0m 0s | | markdownlint was not available. | | +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. | | +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 49 new or modified test files. | _ trunk Compile Tests _ | | +0 :ok: | mvndep | 5m 54s | | Maven dependency ordering for branch | | +1 :green_heart: | mvninstall | 29m 50s | | trunk passed | | +1 :green_heart: | compile | 16m 23s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | compile | 15m 7s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | checkstyle | 4m 20s | | trunk passed | | +1 :green_heart: | mvnsite | 3m 13s | | trunk passed | | +1 :green_heart: | javadoc | 3m 0s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javadoc | 3m 27s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 5m 42s | | trunk passed | | +1 :green_heart: | shadedclient | 33m 2s | | branch has no errors when building and testing our client artifacts. | | -0 :warning: | patch | 33m 28s | | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. | _ Patch Compile Tests _ | | +0 :ok: | mvndep | 0m 32s | | Maven dependency ordering for patch | | +1 :green_heart: | mvninstall | 1m 54s | | the patch passed | | +1 :green_heart: | compile | 15m 51s | | the patch passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javac | 15m 51s | | the patch passed | | +1 :green_heart: | compile | 15m 6s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | javac | 15m 6s | | the patch passed | | +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. | | +1 :green_heart: | checkstyle | 4m 15s | | root: The patch generated 0 new + 1233 unchanged - 16 fixed = 1233 total (was 1249) | | +1 :green_heart: | mvnsite | 3m 11s | | the patch passed | | -1 :x: | javadoc | 1m 19s | [/results-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/6/artifact/out/results-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04.txt) | hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 generated 13 new + 3117 unchanged - 0 fixed = 3130 total (was 3117) | | +1 :green_heart: | javadoc | 3m 24s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 6m 15s | | the patch passed | | +1 :green_heart: | shadedclient | 33m 6s | | patch has no errors when building and testing our client artifacts. | _ Other Tests _ | | +1 :green_heart: | unit | 227m 34s | | hadoop-hdfs in the patch passed. | | +1 :green_heart: | unit | 30m 15s | | hadoop-hdfs-rbf in the patch passed. | | +1 :green_heart: | unit | 0m 59s | | hadoop-fs2img in the patch passed. | | +1 :green_heart: | asflicense | 1m 12s | | The patch does not generate ASF License warnings. | | | | 474m 1s | | | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/6/artifact/out/Dockerfile | | GITHUB PR | https://github.com/apache/hadoop/pull/6762 | | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets xmllint markdownlint | | uname | Linux 77e0fea14a86 5.15.0-124-generic #134-Ubuntu SMP Fri Sep 27 20:20:17 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personalit
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hadoop-yetus commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2561278567 :broken_heart: **-1 overall** | Vote | Subsystem | Runtime | Logfile | Comment | |::|--:|:|::|:---:| | +0 :ok: | reexec | 0m 33s | | Docker mode activated. | _ Prechecks _ | | +1 :green_heart: | dupname | 0m 1s | | No case conflicting files found. | | +0 :ok: | codespell | 0m 1s | | codespell was not available. | | +0 :ok: | detsecrets | 0m 1s | | detect-secrets was not available. | | +0 :ok: | xmllint | 0m 1s | | xmllint was not available. | | +0 :ok: | markdownlint | 0m 1s | | markdownlint was not available. | | +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. | | +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 49 new or modified test files. | _ trunk Compile Tests _ | | +0 :ok: | mvndep | 5m 57s | | Maven dependency ordering for branch | | +1 :green_heart: | mvninstall | 29m 58s | | trunk passed | | +1 :green_heart: | compile | 16m 25s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | compile | 14m 46s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | checkstyle | 4m 22s | | trunk passed | | +1 :green_heart: | mvnsite | 3m 12s | | trunk passed | | +1 :green_heart: | javadoc | 2m 54s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javadoc | 3m 21s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 5m 45s | | trunk passed | | +1 :green_heart: | shadedclient | 33m 16s | | branch has no errors when building and testing our client artifacts. | | -0 :warning: | patch | 33m 42s | | Used diff version of patch file. Binary files and potentially other changes not applied. Please rebase and squash commits if necessary. | _ Patch Compile Tests _ | | +0 :ok: | mvndep | 0m 32s | | Maven dependency ordering for patch | | +1 :green_heart: | mvninstall | 1m 55s | | the patch passed | | +1 :green_heart: | compile | 15m 41s | | the patch passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javac | 15m 41s | | the patch passed | | +1 :green_heart: | compile | 14m 55s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | javac | 14m 55s | | the patch passed | | +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. | | +1 :green_heart: | checkstyle | 4m 19s | | root: The patch generated 0 new + 1233 unchanged - 16 fixed = 1233 total (was 1249) | | +1 :green_heart: | mvnsite | 3m 10s | | the patch passed | | -1 :x: | javadoc | 1m 19s | [/results-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/5/artifact/out/results-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04.txt) | hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 generated 13 new + 3117 unchanged - 0 fixed = 3130 total (was 3117) | | +1 :green_heart: | javadoc | 3m 20s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 6m 18s | | the patch passed | | +1 :green_heart: | shadedclient | 33m 40s | | patch has no errors when building and testing our client artifacts. | _ Other Tests _ | | -1 :x: | unit | 222m 50s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/5/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. | | +1 :green_heart: | unit | 30m 16s | | hadoop-hdfs-rbf in the patch passed. | | +1 :green_heart: | unit | 0m 59s | | hadoop-fs2img in the patch passed. | | +1 :green_heart: | asflicense | 1m 12s | | The patch does not generate ASF License warnings. | | | | 469m 13s | | | | Reason | Tests | |---:|:--| | Failed junit tests | hadoop.hdfs.TestDecommission | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.47 ServerAPI=1.47 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/5/artifact/out/Dockerfile | | GITHUB PR | https://github.com/apache/hadoop/pull/6762 | | Optional Tests | dupname asflicense compile javac javado
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hadoop-yetus commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2553294502 :broken_heart: **-1 overall** | Vote | Subsystem | Runtime | Logfile | Comment | |::|--:|:|::|:---:| | +0 :ok: | reexec | 0m 31s | | Docker mode activated. | _ Prechecks _ | | +1 :green_heart: | dupname | 0m 1s | | No case conflicting files found. | | +0 :ok: | codespell | 0m 1s | | codespell was not available. | | +0 :ok: | detsecrets | 0m 1s | | detect-secrets was not available. | | +0 :ok: | xmllint | 0m 1s | | xmllint was not available. | | +0 :ok: | markdownlint | 0m 1s | | markdownlint was not available. | | +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. | | +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 49 new or modified test files. | _ trunk Compile Tests _ | | +0 :ok: | mvndep | 7m 32s | | Maven dependency ordering for branch | | +1 :green_heart: | mvninstall | 31m 51s | | trunk passed | | +1 :green_heart: | compile | 16m 22s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | compile | 15m 1s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | checkstyle | 4m 26s | | trunk passed | | +1 :green_heart: | mvnsite | 3m 11s | | trunk passed | | +1 :green_heart: | javadoc | 3m 2s | | trunk passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javadoc | 3m 26s | | trunk passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 5m 52s | | trunk passed | | +1 :green_heart: | shadedclient | 35m 5s | | branch has no errors when building and testing our client artifacts. | _ Patch Compile Tests _ | | +0 :ok: | mvndep | 0m 33s | | Maven dependency ordering for patch | | +1 :green_heart: | mvninstall | 1m 59s | | the patch passed | | +1 :green_heart: | compile | 15m 42s | | the patch passed with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 | | +1 :green_heart: | javac | 15m 42s | | the patch passed | | +1 :green_heart: | compile | 14m 52s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | javac | 14m 52s | | the patch passed | | +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. | | -0 :warning: | checkstyle | 4m 23s | [/results-checkstyle-root.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/4/artifact/out/results-checkstyle-root.txt) | root: The patch generated 7 new + 1233 unchanged - 16 fixed = 1240 total (was 1249) | | +1 :green_heart: | mvnsite | 3m 8s | | the patch passed | | -1 :x: | javadoc | 1m 19s | [/results-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/4/artifact/out/results-javadoc-javadoc-hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04.txt) | hadoop-hdfs-project_hadoop-hdfs-jdkUbuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 with JDK Ubuntu-11.0.25+9-post-Ubuntu-1ubuntu120.04 generated 13 new + 3117 unchanged - 0 fixed = 3130 total (was 3117) | | +1 :green_heart: | javadoc | 3m 21s | | the patch passed with JDK Private Build-1.8.0_432-8u432-ga~us1-0ubuntu2~20.04-ga | | +1 :green_heart: | spotbugs | 6m 15s | | the patch passed | | +1 :green_heart: | shadedclient | 33m 28s | | patch has no errors when building and testing our client artifacts. | _ Other Tests _ | | -1 :x: | unit | 220m 51s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/4/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. | | -1 :x: | unit | 30m 0s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs-rbf.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/4/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs-rbf.txt) | hadoop-hdfs-rbf in the patch passed. | | +1 :green_heart: | unit | 0m 53s | | hadoop-fs2img in the patch passed. | | +1 :green_heart: | asflicense | 1m 6s | | The patch does not generate ASF License warnings. | | | | 472m 40s | | | | Reason | Tests | |---:|:--| | Failed junit tests | hadoop.hdfs.TestDecommissionWithBackoffMonitor | | | hadoop.hdfs.TestDecommission | | | hadoop.hdfs.server.federation.router.TestRouterClientRejectOverload | | Subsystem | Report/Notes | |--:|:-| |
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
zhengchenyu commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2141744912 I know this PR is phase 1 work: split global lock into fslock and bmlock. We should ensure the lock order: bmlock cannot occur in front of fslock. Can we add some restrictions or check mechanisms to avoid getting a bmlock before fslock? -- 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
zhengchenyu commented on code in PR #6762: URL: https://github.com/apache/hadoop/pull/6762#discussion_r1622166340 ## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/fgl/FineGrainedFSNamesystemLock.java: ## @@ -0,0 +1,300 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hdfs.server.namenode.fgl; + +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.hdfs.server.namenode.FSNamesystemLock; +import org.apache.hadoop.metrics2.lib.MutableRatesWithAggregation; + +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.function.Supplier; + +/** + * Splitting the global FSN lock into FSLock and BMLock. + * FSLock is used to protect directory tree-related operations. + * BMLock is used to protect block-related and dn-related operations. + * The lock order should be: FSLock,BMLock. + */ +public class FineGrainedFSNamesystemLock implements FSNLockManager { + private final FSNamesystemLock fsLock; + private final FSNamesystemLock bmLock; + + public FineGrainedFSNamesystemLock(Configuration conf, MutableRatesWithAggregation aggregation) { +this.fsLock = new FSNamesystemLock(conf, "FS", aggregation); +this.bmLock = new FSNamesystemLock(conf, "BM", aggregation); + } + + @Override + public void readLock(FSNamesystemLockMode lockMode) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.readLock(); + this.bmLock.readLock(); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readLock(); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readLock(); +} + } + + public void readLockInterruptibly(FSNamesystemLockMode lockMode) throws InterruptedException { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.readLockInterruptibly(); + try { +this.bmLock.readLockInterruptibly(); + } catch (InterruptedException e) { +// The held FSLock should be released if the current thread is interrupted +// while acquiring the BMLock. +this.fsLock.readUnlock("BMReadLockInterruptiblyFailed"); +throw e; + } +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readLockInterruptibly(); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readLockInterruptibly(); +} + } + + @Override + public void readUnlock(FSNamesystemLockMode lockMode, String opName) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.readUnlock(opName); + this.fsLock.readUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readUnlock(opName); +} + } + + public void readUnlock(FSNamesystemLockMode lockMode, String opName, + Supplier lockReportInfoSupplier) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.readUnlock(opName, lockReportInfoSupplier); + this.fsLock.readUnlock(opName, lockReportInfoSupplier); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.readUnlock(opName, lockReportInfoSupplier); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.readUnlock(opName, lockReportInfoSupplier); +} + } + + @Override + public void writeLock(FSNamesystemLockMode lockMode) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.fsLock.writeLock(); + this.bmLock.writeLock(); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeLock(); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.writeLock(); +} + } + + @Override + public void writeUnlock(FSNamesystemLockMode lockMode, String opName) { +if (lockMode.equals(FSNamesystemLockMode.GLOBAL)) { + this.bmLock.writeUnlock(opName); + this.fsLock.writeUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.FS)) { + this.fsLock.writeUnlock(opName); +} else if (lockMode.equals(FSNamesystemLockMode.BM)) { + this.bmLock.wr
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hadoop-yetus commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2106553639 :broken_heart: **-1 overall** | Vote | Subsystem | Runtime | Logfile | Comment | |::|--:|:|::|:---:| | -1 :x: | patch | 1m 02s | | https://github.com/apache/hadoop/pull/6762 does not apply to trunk. Rebase required? Wrong Branch? See https://cwiki.apache.org/confluence/display/HADOOP/How+To+Contribute for help. | | Subsystem | Report/Notes | |--:|:-| | GITHUB PR | https://github.com/apache/hadoop/pull/6762 | | Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch-windows-10/job/PR-6762/3/console | | versions | git=2.44.0.windows.1 | | 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
ZanderXu commented on code in PR #6762: URL: https://github.com/apache/hadoop/pull/6762#discussion_r1591744778 ## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java: ## @@ -6043,7 +6079,7 @@ void updatePipeline( updatePipelineInternal(clientName, oldBlock, newBlock, newNodes, newStorageIDs, logRetryCache); } finally { - writeUnlock("updatePipeline"); + writeUnlock(FSNamesystemLockMode.GLOBAL, "updatePipeline"); Review Comment: Yes, this RPC involves iNode and FSEditLog, so we use global 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
ferhui commented on code in PR #6762: URL: https://github.com/apache/hadoop/pull/6762#discussion_r1590192211 ## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java: ## @@ -6043,7 +6079,7 @@ void updatePipeline( updatePipelineInternal(clientName, oldBlock, newBlock, newNodes, newStorageIDs, logRetryCache); } finally { - writeUnlock("updatePipeline"); + writeUnlock(FSNamesystemLockMode.GLOBAL, "updatePipeline"); Review Comment: That documentation is the final solution. Currently this PR is the 1st stage. This stage only aims to split the global lock into bm and fs locks. In updatePipelineInternal we can see it uses inode too, so we use global lock here. Will solve similar cases in 2nd stage. @ZanderXu any other comments? -- 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hadoop-yetus commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2081720138 :confetti_ball: **+1 overall** | Vote | Subsystem | Runtime | Logfile | Comment | |::|--:|:|::|:---:| _ Prechecks _ | | +1 :green_heart: | dupname | 0m 14s | | No case conflicting files found. | | +0 :ok: | spotbugs | 0m 01s | | spotbugs executables are not available. | | +0 :ok: | codespell | 0m 01s | | codespell was not available. | | +0 :ok: | detsecrets | 0m 01s | | detect-secrets was not available. | | +0 :ok: | xmllint | 0m 01s | | xmllint was not available. | | +0 :ok: | markdownlint | 0m 01s | | markdownlint was not available. | | +1 :green_heart: | @author | 0m 00s | | The patch does not contain any @author tags. | | +1 :green_heart: | test4tests | 0m 00s | | The patch appears to include 47 new or modified test files. | _ trunk Compile Tests _ | | +0 :ok: | mvndep | 2m 18s | | Maven dependency ordering for branch | | +1 :green_heart: | mvninstall | 87m 56s | | trunk passed | | +1 :green_heart: | compile | 39m 10s | | trunk passed | | +1 :green_heart: | checkstyle | 7m 54s | | trunk passed | | +1 :green_heart: | mvnsite | 15m 54s | | trunk passed | | +1 :green_heart: | javadoc | 14m 59s | | trunk passed | | +1 :green_heart: | shadedclient | 174m 23s | | branch has no errors when building and testing our client artifacts. | _ Patch Compile Tests _ | | +0 :ok: | mvndep | 2m 14s | | Maven dependency ordering for patch | | +1 :green_heart: | mvninstall | 9m 51s | | the patch passed | | +1 :green_heart: | compile | 36m 45s | | the patch passed | | +1 :green_heart: | javac | 36m 44s | | the patch passed | | +1 :green_heart: | blanks | 0m 01s | | The patch has no blanks issues. | | +1 :green_heart: | checkstyle | 5m 53s | | the patch passed | | +1 :green_heart: | mvnsite | 16m 04s | | the patch passed | | +1 :green_heart: | javadoc | 14m 57s | | the patch passed | | +1 :green_heart: | shadedclient | 183m 47s | | patch has no errors when building and testing our client artifacts. | _ Other Tests _ | | +1 :green_heart: | asflicense | 6m 03s | | The patch does not generate ASF License warnings. | | | | 552m 56s | | | | Subsystem | Report/Notes | |--:|:-| | GITHUB PR | https://github.com/apache/hadoop/pull/6762 | | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets xmllint markdownlint | | uname | MINGW64_NT-10.0-17763 087b28dd158d 3.4.10-87d57229.x86_64 2024-02-14 20:17 UTC x86_64 Msys | | Build tool | maven | | Personality | /c/hadoop/dev-support/bin/hadoop.sh | | git revision | trunk / 14153f07aab8a229f128f503b2741d1c81a824b2 | | Default Java | Azul Systems, Inc.-1.8.0_332-b09 | | Test Results | https://ci-hadoop.apache.org/job/hadoop-multibranch-windows-10/job/PR-6762/2/testReport/ | | modules | C: hadoop-hdfs-project/hadoop-hdfs hadoop-hdfs-project/hadoop-hdfs-rbf hadoop-tools/hadoop-fs2img U: . | | Console output | https://ci-hadoop.apache.org/job/hadoop-multibranch-windows-10/job/PR-6762/2/console | | versions | git=2.44.0.windows.1 | | 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hfutatzhanghb commented on code in PR #6762: URL: https://github.com/apache/hadoop/pull/6762#discussion_r1578763344 ## hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java: ## @@ -6043,7 +6079,7 @@ void updatePipeline( updatePipelineInternal(clientName, oldBlock, newBlock, newNodes, newStorageIDs, logRetryCache); } finally { - writeUnlock("updatePipeline"); + writeUnlock(FSNamesystemLockMode.GLOBAL, "updatePipeline"); Review Comment: @ZanderXu Hi, sir. Have a question here. In documentaion, it is said that updateBlockForPipeline and updatePipeline are only involved by blocks operations. Why we use FSNamesystemLockMode.GLOBAL 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
ZanderXu commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2071967284 The spotbugs will be fixed by HDFS-17451. And the failed UT is not imported by 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
Re: [PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
hadoop-yetus commented on PR #6762: URL: https://github.com/apache/hadoop/pull/6762#issuecomment-2071959089 :broken_heart: **-1 overall** | Vote | Subsystem | Runtime | Logfile | Comment | |::|--:|:|::|:---:| | +0 :ok: | reexec | 0m 30s | | Docker mode activated. | _ Prechecks _ | | +1 :green_heart: | dupname | 0m 1s | | No case conflicting files found. | | +0 :ok: | codespell | 0m 1s | | codespell was not available. | | +0 :ok: | detsecrets | 0m 1s | | detect-secrets was not available. | | +0 :ok: | xmllint | 0m 1s | | xmllint was not available. | | +0 :ok: | markdownlint | 0m 1s | | markdownlint was not available. | | +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. | | +1 :green_heart: | test4tests | 0m 0s | | The patch appears to include 47 new or modified test files. | _ trunk Compile Tests _ | | +0 :ok: | mvndep | 14m 52s | | Maven dependency ordering for branch | | +1 :green_heart: | mvninstall | 32m 14s | | trunk passed | | +1 :green_heart: | compile | 17m 33s | | trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1 | | +1 :green_heart: | compile | 16m 26s | | trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06 | | +1 :green_heart: | checkstyle | 4m 34s | | trunk passed | | +1 :green_heart: | mvnsite | 3m 19s | | trunk passed | | +1 :green_heart: | javadoc | 3m 0s | | trunk passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1 | | +1 :green_heart: | javadoc | 3m 26s | | trunk passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06 | | -1 :x: | spotbugs | 1m 34s | [/branch-spotbugs-hadoop-hdfs-project_hadoop-hdfs-rbf-warnings.html](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/1/artifact/out/branch-spotbugs-hadoop-hdfs-project_hadoop-hdfs-rbf-warnings.html) | hadoop-hdfs-project/hadoop-hdfs-rbf in trunk has 1 extant spotbugs warnings. | | +1 :green_heart: | shadedclient | 34m 18s | | branch has no errors when building and testing our client artifacts. | _ Patch Compile Tests _ | | +0 :ok: | mvndep | 0m 34s | | Maven dependency ordering for patch | | +1 :green_heart: | mvninstall | 2m 3s | | the patch passed | | +1 :green_heart: | compile | 16m 51s | | the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1 | | +1 :green_heart: | javac | 16m 51s | | the patch passed | | +1 :green_heart: | compile | 16m 21s | | the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06 | | +1 :green_heart: | javac | 16m 21s | | the patch passed | | +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. | | +1 :green_heart: | checkstyle | 4m 36s | | root: The patch generated 0 new + 1232 unchanged - 16 fixed = 1232 total (was 1248) | | +1 :green_heart: | mvnsite | 3m 22s | | the patch passed | | +1 :green_heart: | javadoc | 2m 55s | | the patch passed with JDK Ubuntu-11.0.22+7-post-Ubuntu-0ubuntu220.04.1 | | +1 :green_heart: | javadoc | 3m 32s | | the patch passed with JDK Private Build-1.8.0_402-8u402-ga-2ubuntu1~20.04-b06 | | +1 :green_heart: | spotbugs | 6m 34s | | the patch passed | | +1 :green_heart: | shadedclient | 34m 14s | | patch has no errors when building and testing our client artifacts. | _ Other Tests _ | | -1 :x: | unit | 237m 5s | [/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/1/artifact/out/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt) | hadoop-hdfs in the patch passed. | | +1 :green_heart: | unit | 29m 40s | | hadoop-hdfs-rbf in the patch passed. | | +1 :green_heart: | unit | 0m 50s | | hadoop-fs2img in the patch passed. | | +1 :green_heart: | asflicense | 1m 9s | | The patch does not generate ASF License warnings. | | | | 506m 7s | | | | Reason | Tests | |---:|:--| | Failed junit tests | hadoop.hdfs.server.datanode.TestLargeBlockReport | | Subsystem | Report/Notes | |--:|:-| | Docker | ClientAPI=1.45 ServerAPI=1.45 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6762/1/artifact/out/Dockerfile | | GITHUB PR | https://github.com/apache/hadoop/pull/6762 | | Optional Tests | dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets xmllint markdownlint | | uname | Linux ac7b975b9e42 5.15.0-94-generic #104-Ubuntu SMP Tue Jan 9 15:25:40 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux | | Build tool | maven | | Personality | dev-support/bin/hadoop.sh | | git r
[PR] HDFS-17384. [FGL] Replace the global lock with global FS Lock and global BM lock [hadoop]
ZanderXu opened a new pull request, #6762: URL: https://github.com/apache/hadoop/pull/6762 We plan to merge HDFS-17384 to the trunk branch. This PR is used to review all changes in HDFS-17384. -- 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-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org