HDDS-13602. Delay delete source container replica to avoid read failure due to read thread holds the old containerData #8965
Conversation
|
This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days. |
|
@szetszwo @Gargi-jais11 Please kindly take a look. @ChenSammi Could you help resolve the conflicts? |
|
@ivandika3 , thanks for the checking. I will close this one since agreement of the idea is not reached. |
|
@ChenSammi Thanks for the info. |
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@ChenSammi , on a second thought, delaying deletion is a good workaround for the race condition. Let's reopen this?
Please see also the comments inlined.
| private void cleanupPendingDeletionContainers() { | ||
| // delete all pending deletion containers before stop the service | ||
| // we still need to honor the expiry time of deletion | ||
| Iterator<Map.Entry<Long, Container>> iterator = pendingDeletionContainers.entrySet().iterator(); | ||
| while (iterator.hasNext()) { | ||
| Map.Entry<Long, Container> entry = iterator.next(); | ||
| if (entry.getKey() <= System.currentTimeMillis()) { | ||
| iterator.remove(); | ||
| deleteContainer(entry.getValue()); | ||
| } else { | ||
| // Since the map is sorted by key (timestamp), we can break early. | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
We may use headMap.
Actually, let's make tryCleanupOnePendingDeletionContainer return boolean and then reuse it here.
private void cleanupPendingDeletionContainers() {
for (; tryCleanupOnePendingDeletionContainer(); ) ;
}
private boolean tryCleanupOnePendingDeletionContainer() {
final Map.Entry<Long, Container> entry = pendingDeletionContainers.pollFirstEntry();
if (entry != null) {
if (entry.getKey() <= System.currentTimeMillis()) {
// entry container is expired
deleteContainer(entry.getValue());
return true;
}
// put back the container
pendingDeletionContainers.put(entry.getKey(), entry.getValue());
}
return false;
}| if (!pendingDeletionContainers.isEmpty()) { | ||
| Map.Entry<Long, Container> entry = pendingDeletionContainers.pollFirstEntry(); | ||
| if (entry != null) { |
There was a problem hiding this comment.
Checking isEmpty() is not needed since pollFirstEntry() returns null if the map is empty.
ChenSammi
force-pushed
the
HDDS-13602-lazy-delete
branch
2 times, most recently
from
e2cf33d to
d755a41
Compare
…re due to read thread holds the old containerData
ChenSammi
force-pushed
the
HDDS-13602-lazy-delete
branch
from
d755a41 to
831eaee
Compare
|
@szetszwo thanks for the review. Could you like to take another look? |
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
+1 the change looks good.
|
Thanks @szetszwo for the review. |
3 participants
What changes were proposed in this pull request?
delay the deletion of old replica
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13602
How was this patch tested?
new unit test