[
https://issues.apache.org/jira/browse/HDFS-16819?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17626547#comment-17626547
]
ASF GitHub Bot commented on HDFS-16819:
---------------------------------------
haiyang1987 commented on PR #5074:
URL: https://github.com/apache/hadoop/pull/5074#issuecomment-1296974063
> I discussed with @haiyang1987 yesterday, this PR doesn't make sense. There
are some bugs in the `createTemporary` method.
>
> @haiyang1987 Will you modify this PR to describe and fix them?
Sure, I will improve it later.
Thanks @ZanderXu @tomscut help to review.
> Remove the redundant write lock in FsDatasetImpl#createTemporary
> ------------------------------------------------------------------
>
> Key: HDFS-16819
> URL: https://issues.apache.org/jira/browse/HDFS-16819
> Project: Hadoop HDFS
> Issue Type: Improvement
> Reporter: Haiyang Hu
> Assignee: Haiyang Hu
> Priority: Major
> Labels: pull-request-available
>
> In FsDatasetImpl#createTemporary Line_1840 the writeLock here seems useless.
> The readLock is already held in volumeMap.get(). From the code logic point
> of view, the writeLock here maybe to remove
> {code:java}
> public ReplicaHandler createTemporary(StorageType storageType,
> String storageId, ExtendedBlock b, boolean isTransfer)
> throws IOException {
> long startTimeMs = Time.monotonicNow();
> long writerStopTimeoutMs = datanode.getDnConf().getXceiverStopTimeout();
> ReplicaInfo lastFoundReplicaInfo = null;
> boolean isInPipeline = false;
> do {
> try (AutoCloseableLock lock = lockManager.writeLock(LockLevel.BLOCK_POOl,
> b.getBlockPoolId())) { //the writeLock here maybe to remove
> ReplicaInfo currentReplicaInfo =
> volumeMap.get(b.getBlockPoolId(), b.getBlockId());
> if (currentReplicaInfo == lastFoundReplicaInfo) {
> break;
> } else {
> isInPipeline = currentReplicaInfo.getState() == ReplicaState.TEMPORARY
> || currentReplicaInfo.getState() == ReplicaState.RBW;
> /*
> * If the current block is not PROVIDED and old, reject.
> * else If transfer request, then accept it.
> * else if state is not RBW/Temporary, then reject
> * If current block is PROVIDED, ignore the replica.
> */
> if (((currentReplicaInfo.getGenerationStamp() >= b
> .getGenerationStamp()) || (!isTransfer && !isInPipeline))
> && !isReplicaProvided(currentReplicaInfo)) {
> throw new ReplicaAlreadyExistsException("Block " + b
> + " already exists in state " + currentReplicaInfo.getState()
> + " and thus cannot be created.");
> }
> lastFoundReplicaInfo = currentReplicaInfo;
> }
> }
> {code}
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
