[
https://issues.apache.org/jira/browse/HDFS-11248?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15763326#comment-15763326
]
Uma Maheswara Rao G commented on HDFS-11248:
--------------------------------------------
[~rakeshr], Thank you for accepting the idea and providing the patch quickly.
Please find the feed back on latest patch.
# -
I think there is race condition here in the following condition:
{code}
synchronized (storageMovementAttemptedResults) {
boolean exist = isExistInResult(blockCollectionID);
if (!exist) {
blockStorageMovementNeeded.add(blockCollectionID);
} else {
LOG.info("Blocks storage movement results for the"
+ " tracking id : " + blockCollectionID
+ " is reported from one of the co-ordinating datanode."
+ " So, the result will be processed soon.");
}
iter.remove();
}
{code}
Talking about partial block movements case.
Lets assume this piece has got chance to execute when immediately after SUCCESS
result came.
So, storageMovementAttemptedResults contains the element, but not processed
yet. The above condition thinks exist as true and will not add element back to
blockStorageMovementNeeded for actual retry. Now assume, Later following code
got chance to execute. itemInfo.isAllBlockLocsAttemptedToSatisfy() will be true
and will try to remove from storageMovementAttemptedItems. But here with the
current patch, the element disappeared in all the lists now. So, we can not
retry.
So, Here based on
result(storageMovementAttemptedItems.remove(storageMovementAttemptedResult.getTrackId())
) you may need to act. if remove is true, then fine, otherwise
storageMovementAttemptedItems item would have processed earlier than the
current processing. You can rethink better in this conditions may be.
{code}
synchronized (storageMovementAttemptedItems) {
ItemInfo itemInfo = storageMovementAttemptedItems
.get(storageMovementAttemptedResult.getTrackId());
if (itemInfo.isAllBlockLocsAttemptedToSatisfy()) {
storageMovementAttemptedItems
.remove(storageMovementAttemptedResult.getTrackId());
}
}
{code}
# -
I would like you to refine log message to represent exact situation here.
{code}
LOG.warn("Blocks storage movement results for the tracking id : "
+ storageMovementAttemptedResult.getTrackId()
+ " is reported from co-ordinating datanode, but result"
+ " status is FAILURE. So, added for retry");
} else {
synchronized (storageMovementAttemptedItems) {
ItemInfo itemInfo = storageMovementAttemptedItems
.get(storageMovementAttemptedResult.getTrackId());
if (itemInfo.isAllBlockLocsAttemptedToSatisfy()) {
storageMovementAttemptedItems
.remove(storageMovementAttemptedResult.getTrackId());
}
}
LOG.info("Blocks storage movement results for the tracking id : "
+ storageMovementAttemptedResult.getTrackId()
+ " is reported from co-ordinating datanode. "
+ "The result status is SUCCESS.”);
{code}
When SUCCESS but itemInfo.isAllBlockLocsAttemptedToSatisfy is false
, you may not remove from storageMovementAttemptedItems. This means you are
retrying. May be we can say, result success but will retry due to this
condition.
# -
{code}
if (!blockCollection.getLastBlock().isComplete()) {
// Postpone, currently file is under construction
// So, should we add back? or leave it to user
- return;
+ return true;
{code}
Not introduced from this patch, but for improving debug-ability we can add
message here why we are postponing.
> [SPS]: Handle partial block location movements
> ----------------------------------------------
>
> Key: HDFS-11248
> URL: https://issues.apache.org/jira/browse/HDFS-11248
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: datanode, namenode
> Affects Versions: HDFS-10285
> Reporter: Rakesh R
> Assignee: Rakesh R
> Attachments: HDFS-11248-HDFS-10285-00.patch,
> HDFS-11248-HDFS-10285-01.patch
>
>
> This jira is to handle partial block location movements due to unavailability
> of target nodes for the matching storage type.
> For example, We have only A(disk,archive), B(disk) and C(disk,archive) are
> live nodes with A & C have archive storage type. Say, we have a block with
> locations {{A(disk), B(disk), C(disk)}}. Again assume, user changed the
> storage policy to COLD. Now, SPS internally starts preparing the src-target
> pairing like, {{src=> (A, B, C) and target=> (A, C)}} and sends
> BLOCK_STORAGE_MOVEMENT to the coordinator. SPS is skipping B as it doesn't
> have archive media to indicate that it should do retries to satisfy all block
> locations after some time. On receiving the movement command, coordinator
> will pair the src-target node to schedule actual physical movements like,
> {{movetask=> (A, A), (B, C)}}. Here ideally it should do {{(C, C)}} instead
> of {{(B, C)}} but mistakenly choosing the source C and creates problem.
> IMHO, the implicit assumptions of retry needed is creating confusions and
> leads to coding mistakes. One idea to fix this problem is to create a new
> flag {{retryNeeded}} flag to make it more readable. With this, SPS will
> prepare only the matching pair and dummy source slots will be avoided like,
> {{src=> (A, C) and target=> (A, C)}} and mark {{retryNeeded=true}} to convey
> the message that this {{trackId}} has only partial blocks movements.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
