Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-444991 Thanks @ChenSammi, @sodonnel and @adoroszlai for reviewing the patch. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 merged PR #10109: URL: https://github.com/apache/ozone/pull/10109 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4449406190 @sodonnel would you like to take another look? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3240181171
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##
@@ -2293,24 +2306,33 @@ private boolean logBlocksFoundOnDisk(Container
container) throws IOException {
private void deleteInternal(Container container, boolean force)
throws StorageContainerException {
+final long containerId = container.getContainerData().getContainerID();
long startTime = clock.millis();
-container.writeLock();
+Container containerLocked =
containerSet.acquireContainerLock(containerId);
+if (containerLocked == null) {
+ // null means container locking retries exhausted ;
+ // container not-found throws StorageContainerException.
+ LOG.info("Exceeded {} retries to lock container {}; Now DN will resend
for delete with " +
Review Comment:
Yes
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r324017
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##
@@ -2293,24 +2306,33 @@ private boolean logBlocksFoundOnDisk(Container
container) throws IOException {
private void deleteInternal(Container container, boolean force)
throws StorageContainerException {
+final long containerId = container.getContainerData().getContainerID();
long startTime = clock.millis();
-container.writeLock();
+Container containerLocked =
containerSet.acquireContainerLock(containerId);
+if (containerLocked == null) {
+ // null means container locking retries exhausted ;
+ // container not-found throws StorageContainerException.
+ LOG.info("Exceeded {} retries to lock container {}; Now DN will resend
for delete with " +
Review Comment:
Is this one addressed?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r324017
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##
@@ -2293,24 +2306,33 @@ private boolean logBlocksFoundOnDisk(Container
container) throws IOException {
private void deleteInternal(Container container, boolean force)
throws StorageContainerException {
+final long containerId = container.getContainerData().getContainerID();
long startTime = clock.millis();
-container.writeLock();
+Container containerLocked =
containerSet.acquireContainerLock(containerId);
+if (containerLocked == null) {
+ // null means container locking retries exhausted ;
+ // container not-found throws StorageContainerException.
+ LOG.info("Exceeded {} retries to lock container {}; Now DN will resend
for delete with " +
Review Comment:
Is this one updated?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4449041914 And for keyValueHandler.quasiCloseContainer this also does not require refetching as this will only be called when container state is in CLOSING. DiskBalancer avoids CLOSING so no updation of container set, no race. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4448897209 KeyValueHandler.markContainerForClosed need be covered too. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3239767828
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##
@@ -2293,24 +2306,33 @@ private boolean logBlocksFoundOnDisk(Container
container) throws IOException {
private void deleteInternal(Container container, boolean force)
throws StorageContainerException {
+final long containerId = container.getContainerData().getContainerID();
long startTime = clock.millis();
-container.writeLock();
+Container containerLocked =
containerSet.acquireContainerLock(containerId);
+if (containerLocked == null) {
+ // null means container locking retries exhausted ;
+ // container not-found throws StorageContainerException.
+ LOG.info("Exceeded {} retries to lock container {}; Now DN will resend
for delete with " +
Review Comment:
DN -> SCM
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3239723134
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,54 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped, unlocks and
retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if the mapping could not be
stabilized after all retries
+ * @throws StorageContainerException with {@code CONTAINER_NOT_FOUND}
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) throws
StorageContainerException {
Review Comment:
acquireContainerLock -> getContainerWithWriteLock
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4447638015 @ChenSammi - I have reverted the commit and kept retry approach. - Also improved the container not found and exhausted retry case. Now in case of container not found StorageContainerException CONTAINER_NOT_FOUND is thrown. And if `acquireContainerLock` throws null then it means retry has exhausted. - Retry is reduced to 5. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3234198420
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
For now, I don't see any problem of the new approach. But consider future, I
agree the previous approach is more safe. @Gargi-jais11 , you can use the
previous approach. We should consider reduce the retry count, 10 is too much.
And currently for container not found, and try exhausted case, both return
null, which can be improved.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233543415
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
@sodonnel If you are thinking for future unrelated changes similar to
diskbalancer then yes what u are saying is definitely correct and keeping retry
logic is correct way to avoid this fix fail.
@ChenSammi what do you say? I have already applied your logic may be I will
revert this once decided.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233543415
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
If you are thinking for future unrelated changes similar to diskbalancer
then yes what u are saying is definitely correct and keeping retry logic is
correct way to avoid this fix fail.
@ChenSammi what do you say? I have already applied your logic may be I will
revert this once decided.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233543415
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
If you thinking for future unrelated changes similar to diskbalancer then
yes what u are saying is definitely correct and keeping retry logic is correct
way to avoid this fix fail.
@ChenSammi what do you say?
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
If you are thinking for future unrelated changes similar to diskbalancer
then yes what u are saying is definitely correct and keeping retry logic is
correct way to avoid this fix fail.
@ChenSammi what do you 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233508666
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
> but for now there is only diskbalancer which will swap container in
in-memory.
Until some other change happens in an unrelated area and causes this fix to
break. If there is any chance for the locking to let a race condition in, it is
not correct. The retry loop guaranteed correctness, this new way does not as
far as I can see.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233389538
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
@sodonnel but for now there is only diskbalancer which will swap container
in in-memory. for one diskbalancer task it operates only one time on that
container. It's very rarest case that the same container will be eligible to
move to other disk again but that too will happen in next iteration of balancer
and since we already writeLock wound be working on current then diskbalancer
won't able to acquire readLock again on same container.
considering the above I don't think there is any gap left for this. Please
correct me if I am wrong.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233389538
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
@sodonnel but for now there is only diskbalancer which will swap container.
for one diskbalancer task it operates on only one time on that container. Its
very rarest case that the same container will be eligible to move to other disk
again but that too will happen in next iteration of balancer.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233277722
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
Hmm, makes sense.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233389538
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
@sodonnel but for now there is only diskbalancer which will swap container.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233368271
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
> You can first lock the current, then unlock the candidate, so no retry is
needed.
Are you sure that will work? What if someone swaps the current again while
you are trying to get the lock? You need to refetch and check again:
1. Get container
2. Lock it
3. Check get it again and check it has not changed.
If changed:
1. Lock new copy
2. Need to check if it has changed while waiting for the lock, so its kind
of back to the start.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233277722
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
Hmm, makes sense.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3233062009
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
You can first lock the current, then unlock the candidate, so no retry is
needed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3232317054
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
Sammi, initially we were simply refetching the container and checking with
the existing container if it differs we would fail but then Stephen suggested
its better to refetch the container and retry while acquiring the writeLock so
that we don't rely on RM to resend the same command after this fail.
We hold writeLock on candidate (old object), not on current (new object).
Returning current bare would hand an unlocked container to the caller. We must
first writeUnlock(candidate) then writeLock(current) — but between those two
steps the map can change again, so the loop retries until we lock whatever is
currently live in the map.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3232229673
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
Why it cannot simply return the refetched container after the lock?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3231930084
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##
@@ -303,19 +303,31 @@ public DeleteBlockTransactionExecutionResult call() {
if (keyValueContainer.
writeLockTryLock(tryLockTimeoutMs, TimeUnit.MILLISECONDS)) {
try {
- String schemaVersion = containerData
- .getSupportedSchemaVersionOrDefault();
- if (getSchemaHandlers().containsKey(schemaVersion)) {
-schemaHandlers.get(schemaVersion).handle(containerData, tx);
+ // Re-fetch the container after acquiring the lock. DiskBalancer
may have relocated
+ // this container to a different disk while we waited — in that
case, the container
+ // object in ContainerSet has changed and containerData points
to the old replica.
+ Container current = containerSet.getContainer(containerId);
+ if (current == null || current.getContainerData() !=
containerData) {
+LOG.debug("DeleteBlocks: containerData for container {} is
stale "
++ ", Will retry on the new replica.",
+containerId);
+lockAcquisitionFailed = true;
Review Comment:
Since the retry pass itself always goes through **writeLockTryLock**, a
stale-mapping failure surviving both the first attempt and the retry would
require DiskBalancer to move the same container a second time within the single
retry window — essentially impossible in practice. Adding a second boolean to
separate these cases introduces complexity without any real benefit. Keeping
the single lockAcquisitionFailed flag for both "try-lock timed out" and "stale
mapping after lock" is sufficient, and both correctly trigger the existing
single retry.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3231896371
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
“Retry” here means we redo the acquire loop: unlock the stale object we
locked (no longer equals getContainer(id) after a concurrent updateContainer),
then fetch the map again and writeLock the current entry. We’re not assuming
the replica “swaps back”; we’re handling the race where the id → container
mapping changed between the first lookup and verifying under lock.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Copilot commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3231756482
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##
@@ -303,19 +303,31 @@ public DeleteBlockTransactionExecutionResult call() {
if (keyValueContainer.
writeLockTryLock(tryLockTimeoutMs, TimeUnit.MILLISECONDS)) {
try {
- String schemaVersion = containerData
- .getSupportedSchemaVersionOrDefault();
- if (getSchemaHandlers().containsKey(schemaVersion)) {
-schemaHandlers.get(schemaVersion).handle(containerData, tx);
+ // Re-fetch the container after acquiring the lock. DiskBalancer
may have relocated
+ // this container to a different disk while we waited — in that
case, the container
+ // object in ContainerSet has changed and containerData points
to the old replica.
+ Container current = containerSet.getContainer(containerId);
+ if (current == null || current.getContainerData() !=
containerData) {
+LOG.debug("DeleteBlocks: containerData for container {} is
stale "
++ ", Will retry on the new replica.",
+containerId);
+lockAcquisitionFailed = true;
Review Comment:
`lockAcquisitionFailed` is now also set when a stale container mapping is
detected (after the write lock is acquired). `executeCmdWithRetry` treats any
remaining `lockAcquisitionFailed` after the single retry as a lock-timeout and
increments `incrTotalLockTimeoutTransactionCount()`, which can misclassify
“mapping swapped” cases as timeouts. Consider distinguishing retry reasons (eg,
separate flag/enum for stale-mapping vs tryLock-timeout) so the timeout metric
stays accurate.
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
Review Comment:
`acquireContainerLock` logs at INFO when a container is missing / removed
during lock acquisition. This method is likely called from background tasks
(block deletion, delete commands, etc.), so INFO-level logging here may be
noisy during normal churn (eg, containers concurrently deleted). Consider
lowering these messages to DEBUG (or rate-limiting) to avoid log spam in
production.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3231715913
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
Why retry? The container will swap back 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3231715913
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerSet.java:
##
@@ -279,6 +285,49 @@ public Container getContainer(long containerId) {
return containerMap.get(containerId);
}
+ /**
+ * Returns the max retry for a container map swap while acquiring container
lock.
+ * @return max retry count
+ */
+ public static int maxContainerMapSwapRetries() {
+return MAX_CONTAINER_MAP_SWAP_RETRIES;
+ }
+
+ /**
+ * Locks the container mapped to {@code containerId} for write, and verifies
that the instance locked is still
+ * the one stored in this set. If the mapping is swapped or the container no
longer exists, unlocks and retries up to
+ * {@link #maxContainerMapSwapRetries()} times, then returns {@code null}.
+ *
+ * @return the locked container, or {@code null} if none mapped, or mapping
could not be stabilized
+ */
+ @Nullable
+ public Container acquireContainerLock(long containerId) {
+for (int retry = 0; retry < MAX_CONTAINER_MAP_SWAP_RETRIES; retry++) {
+ Container candidate = getContainer(containerId);
+ if (candidate == null) {
+LOG.info("Container {} no longer present in ContainerSet, skipping.",
containerId);
+return null;
+ }
+ candidate.writeLock();
+ Container current = getContainer(containerId);
+ if (current == null) {
+candidate.writeUnlock();
+LOG.info("Container {} no longer exists in ContainerSet while
acquiring lock.", containerId);
+return null;
+ }
+ if (current != candidate) {
+candidate.writeUnlock();
+if (LOG.isDebugEnabled()) {
+ LOG.debug("Container {} mapping changed during lock acquisition
(attempt {}); retrying.",
Review Comment:
What's the meaning of retry? The container will swap back 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4429223910 Just proceed with the re-fetch idea. The architecture of the code in the datanode is so bad, its impossible to fix this any other way. I still think this leaves the door open for similar bugs in the future as its far from obvious on how this should be used if a new service comes along. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4428934823 > > In-place updates to several fields are atomic under the write lock > > It cannot be under the write lock. If write lock is feasible, this jira is no longer needed. When I said in-place updates to several fields are atomic under the write lock, I meant only the phase where we would swap **metadataPath / chunksPath / volume (etc.) on the existing object** — not the full DiskBalancer copy. The entire copy obviously cannot run under the container write lock; that was never the proposal. You’re still right about the harder issue: many call sites read `KeyValueContainerData` paths (and related state) without holding the container read lock. So even if those field writes are done under writeLock(), we don’t get a clean story for unsynchronized readers (visibility / torn reads). That’s a big part of why the in-place approach felt fragile in practice. Given that, I’m staying with the re-fetch / acquireContainerLock + retry direction for this JIRA instead of mutating paths in place on the live object. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4428752613 > In-place updates to several fields are atomic under the write lock It cannot be under the write lock. If write lock is feasible, this jira is no longer needed. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4427909615 @sodonnel @ChenSammi Yesterday night I tried implementing above diskbalancer flow but it seems to be complicated and bug prone. **(save old paths → readUnlock → writeLock → update volume/metadata/chunks/db paths → unlock → cleanup Disk1 from saved locals)**. _@sodonnel direction is right in principle: one live Container object means no more updateContainer-style staleness for anything that always goes through getContainer(id) and the container lock._ In practice I found it quite easy to get wrong, especially **Phase C (marking/deleting the old replica):** the live object must never be used for Disk1 cleanup, V3 DB/cache handling needs care, crash recovery between phases has to be spelled out, and the r**eadUnlock → writeLock** gap still needs a post-lock state check . Separately, @ChenSammi already called out the bigger issue: many call sites read `KeyValueContainerData` paths without holding the container lock. In-place updates to several fields are atomic under the write lock, but not automatically safe for those **unsynchronized readers** unless we address visibility and torn reads (e.g. volatile/immutable snapshot) and review a lot of DN code — so the “fix in one place” story is not complete without that wider work. I’m leaning to keep the re-fetch/retry fix for this staleness issue. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4421721539 > > Generally we should choose the one with less complexity, and less cost. If replacing the fields inline, there are at least 4 fields' swaps, one by one. If replacing the container reference, there is one swap. Basically, hot swap one pointer has less complex result than hot swap four pointers, right? > > I don't think updating fields in an object, which can be done atomically under an existing lock is complex. > > Swapping the object for another is simple, but look at this bug it causes and the fix which is bleeding out into lots of code areas. And even after this fix, if someone adds a new service, they have to know they need to do these strange things (get a container, then get it again and check it hasn't changed) to avoid a race condition they shouldn't really be concerned about. > > If we fix this at source, by locking the container object, updating the fields atomically and unlocking, then its fixed once and for all in a single place @sodonnel , currently we swap one container pointer under container's read lock protection, we got one stale container reference. If we swap four fields under the same container's read lock protection, we got four stale fields reference. We need to double check all code places where these four stale fields reference can happen to know how many new race conditions and what's their consequences, they might be less severe or more severe than this jira. For example, the chunk read doesn't hold any container lock, once the thread has the container object, it will assume volume, chunk, db path will be not changed. If multiple fields are swapped, it could end up with some path point to new location, some path point to old location, which seems more dangerous than the issues described by this jira, which are mainly there could be some blocks left in the replica, or some replicas are left not deleted in the first place(I believe this replica will be eventually deleted, a new replica for a deleted container will be deleted eventually). And even if we implement the swap four fields solution, it's also not once and for all, every new code added, we also need to verify if there is no stale reference case. The root cause of issue described in this JIRA, is because we do the container pointer swap in containerSet under a read lock protection after container is moved to destination volume, if it's under a write lock protection, then some blocks left in the replica, or some replicas are left not deleted in the first place will not happen. But consider moving a container from one volume might take a substantial time, we cannot afford to write lock the container, as it will make other threads waiting for operation on this container wait for a considerable amount of time. The current approach in this jira, is not a perfect approach, the perfect approach is use write lock, but it's a approach with relative low complexity, as double check after get lock is common in concurrent environment, easy to understand, and it's easy to find out where the write lock is held, and do the double check after the lock acquired, which is easier than identify all the places where volume, db, chunk path, meta path are used, and handle the stale reference. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4417703728 > What about DeleteBlocksCommandHandler, which has this code which locks the Container Object AFTER the containerData object has already been extracted from it and then proceeds to pass that down into lower methods to change it. Thanks @sodonnel for catching this. It seems I missed this handler. Yes the sam erace is applied over here we need to refetch here as well. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on PR #10109:
URL: https://github.com/apache/ozone/pull/10109#issuecomment-4379142286
A problem with enforcing atomic update of the containerData within a
Container object, is that the Container object has the RW lock, and it allows
for the ContainerData object to be fetched without a lock. Then the
ContainerData object can also be mutated without a lock, or mutated by another
thread even when the Container object is locked.
What about DeleteBlocksCommandHandler, which has this code which locks the
Container Object AFTER the containerData object has already been extracted from
it and then proceeds to pass that down into lower methods to change it.
```
case KeyValueContainer:
KeyValueContainer keyValueContainer = (KeyValueContainer)cont;
KeyValueContainerData containerData =
keyValueContainer.getContainerData();
if (keyValueContainer.
writeLockTryLock(tryLockTimeoutMs, TimeUnit.MILLISECONDS)) {
try {
String schemaVersion = containerData
.getSupportedSchemaVersionOrDefault();
if (getSchemaHandlers().containsKey(schemaVersion)) {
schemaHandlers.get(schemaVersion).handle(containerData, tx);
} else {
throw new UnsupportedOperationException(
"Only schema version 1,2,3 are supported.");
}
} finally {
keyValueContainer.writeUnlock();
}
txResultBuilder.setContainerID(containerId)
.setSuccess(true);
} else {
lockAcquisitionFailed = true;
txResultBuilder.setContainerID(containerId)
.setSuccess(false);
}
break;
```
This feels very fragile too and we haven't made any changes to this class -
does it need the same check after lock?
It feels like all these container data fields should be retrieved though the
Container object and that is should check a valid lock is in place before
returning it.
The problem is, containerData is used in 100's of places across the code
base - mostly in tests but over 100 in non-test code too.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4378583519 > Generally we should choose the one with less complexity, and less cost. If replacing the fields inline, there are at least 4 fields' swaps, one by one. If replacing the container reference, there is one swap. Basically, hot swap one pointer has less complex result than hot swap four pointers, right? I don't think updating fields in an object, which can be done atomically under an existing lock is complex. Swapping the object for another is simple, but look at this bug it causes and the fix which is bleeding out into lots of code areas. And even after this fix, if someone adds a new service, they have to know they need to do these strange things (get a container, then get it again and check it hasn't changed) to avoid a race condition they shouldn't really be concerned about. If we fix this at source, by locking the container object, updating the fields atomically and unlocking, then its fixed once and for all in a single place -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4377076006 @ChenSammi @sodonnel Please take a look, I have updated all the possible places where the race can happen. I am continuing my research in this part and if any further things come out we can take that in a followup task. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4350091547 Yaa correct @ChenSammi. I am working on other parts of datanodes to analyse where all issues due to staleness can happen and then we can update them taking in bunches if its more. Will update with the complete analysis of dn here once I am done, need some 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4349940327 @sodonnel, Replace the fields in place in container, or replace container reference in containerMap, these are the two solutions. The one with less complexity, and less cost is preferred. @Gargi-jais11 , please investigate all existing code places, where it needs MetadataPath, ChunksPath, Volume info, RocksDB for it work, what's the impact if the info is swapped in between, and how many edge cases will be there. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4349763866 > What is we instead updated the existing object in place under a lock? Would that solve all these edge cases more simple? It sounds like these problems all come from the disk balancer moving and replacing the container object, so if we fix just that, it may be a cleaner solution to this whole problem? @sodonnel I believe your suggestion is correct and cleaner. **Updating fields in-place wipes out the path to Disk1.** So to resolve this what I can think of is we can save the old path in local variables before acquiring the write lock and updating existing container object. The lock only needs to protect the swap of the live in-memory fields; path cleanup uses the saved local. So according to this the new proposed diskbalancer flow will look something like this: ``` Phase A: File-level copy (identical to today — no change here) A1. container.readLock() A2. copyContainer(Disk1 → tmp/) < files copied A3. atomic Files.move(tmp → Disk2/final) < .container YAML on Disk2 is final Phase B: In-place update (replaces current updateContainer call) B1. Save: oldMeta = containerData.getMetadataPath() oldChunks = containerData.getChunksPath() oldVolume = containerData.getVolume() oldDbFile = containerData.getDbFile() B2. container.readUnlock() B3. container.writeLock() < escalate, blocks all concurrent readers/writers B4. containerData.setVolume(destVolume) containerData.setMetadataPath(newMetadataPath) containerData.setChunksPath(newChunksPath) containerData.setDbFile(newDbFile) B5. container.writeUnlock() Phase C: Disk1 cleanup (uses saved locals — no reference to Disk1 survives in the container object) C1. update volume space: destVolume.incrementUsed; oldVolume.decrementUsed C2. markContainerForDelete on Disk1 using oldMeta/oldChunks/oldDbFile paths C3. delete Disk1 files ``` cc: @ChenSammi Please go through these steps and comment if any issue would be there as part of diskbalancer if we update existing object. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4349672836 > For EC Reconstruction I don't think it exports the entire container like this. More it reads the blocks out of the container block by block using the normal read path through the datanode. @sodonnel My bad I got diverted while exploring. You are correct for ECReconstruction, it reads chunk by chunk per block, over gRPC, using GetBlock + ReadChunk RPCs. The data flows from the source DN's normal read handler **(HddsDispatcher → KeyValueHandler.handleReadChunk)** which does a read lock per chunk, not a write lock. > So the question is - can the normal read path be impacted by the balancer moving a container from disk 1 to disk 2? - DiskBalancer holds readLock during the copy. A concurrent **ReadChunk or GetBlock** call for the same container also needs readLock — multiple readers allowed simultaneously. So they don't block each other. Reads are safe during DB's copy phase. - After **updateContainer**, the ContainerSet maps to the new object. The read handler calls **containerSet.getContainer(id)** fresh on every RPC — it gets the NEW object pointing to Disk2. Reads proceed on Disk2. Safe after the swap. - The only window: if the gRPC channel had fetched the container reference before updateContainer, it still reads from Disk1 files via the OLD object. Since read lock doesn't conflict with another read lock, reads work fine. Disk1 files still exist until **replicaDeletionDelay**. Safe, but again there can be issue if ECReconstruction has still not completed from old container which will be deleted eventually after 5mins of movement to new disk then read failure will happen for ECReconstruction. > What scenarios replace the container object in ContainerSet? What I can see from the codebase is : ``` addContainer() → called at DN startup (ContainerReader), incoming replication (importContainer), EC reconstruction target creation updateContainer() → called ONLY by DiskBalancerService (line 557) ``` That's it. `updateContainer` for diskBalancer is the only production path that swaps the live container object. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4345133119 Looking at the containerSet class, the `updateContainer()` method was added only for Diskbalancer and is only called by disk balancer. The only other place items are added to the `containerMap` is in the `addContainer()` method, so it seems like this could all be solved by updating the `Container` object fields rather than creating a new instance and replacing it. It doesn't appear to be an immutable object so this would be a much smaller fix and **I think** will solve all the 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4345047536 This problem is kind of getting difficult to see if we have solved all the cases or not. The root cause is poor interface design, and further inadequate locking. Fixing this is going to be difficult. The locking is only inadequate because the lock is inside the container object and the container object can be replaced by a new object against the same ID. What are all the scenarios that replace that container object? What is we instead updated the existing object in place under a lock? Would that solve all these edge cases more simple? It **sounds like** these problems all come from the disk balancer moving and replacing the container object, so if we fix just that, it may be a cleaner solution to this whole problem? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4345015157 > In exportContainerData, after acquiring the lock, re-fetch the container from ContainerSet by ID, so even if the old container is marked DELETED the new container moved by diskbalancer will be fetched and will not fail ECReconstruction. For EC Reconstruction I don't think it exports the entire container like this. More it reads the blocks out of the container block by block using the normal read path through the datanode. So the question is - can the normal read path be impacted by the balancer moving a container from disk 1 to disk 2? Ideally, the DN should be safe to do this using its locking as it is not idea that reads would fail randomly. If its Ratis, it can just try another replica with s small delay. With EC it would fall back to reconstruction reads, which are slower and use more resources. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4341551978 @sodonnel @ChenSammi above are more new areas which need same fix. Rest all do not have any issue like this. I have done thorough code checking. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4341547514 for markContainerForUnhealthy with DiskBalancer parallely working . I believe this also needs to refetch the container after writeLock. Here it works for any container state to mark all — CLOSED, QUASI_CLOSED, OPEN, RECOVERING — all can become UNHEALTHY. ``` Case 1: Container CLOSED/QUASI_CLOSED + DiskBalancer + Scanner in parallel DiskBalancer (DN1) Container Scanner (DN1) ── ── T1: selects C (CLOSED, Disk1) added to inProgressContainers T2: container.readLock() on OLD T3: copy Disk1 → Disk2 scanner reads OLD files on Disk1 (I/O in progress) finds checksum failure controller.markContainerUnhealthy(id, reason) containerSet.getContainer(id) → OLD container (Disk1) ← stale ref handler.markContainerUnhealthy(OLD, reason) → writeLock() → BLOCKED (readLock held) T4: copy done, checksum verified T5: importContainer → NEW (CLOSED, Disk2) T6: containerSet.updateContainer(NEW) ← ContainerSet maps C → NEW (Disk2) T7: readUnlock() on OLD writeLock ACQUIRED on OLD state = CLOSED, not UNHEALTHY, volume not failed OLD: CLOSED → UNHEALTHY ← wrong container writeUnlock sendICR: C on DN1 = UNHEALTHY ← stale, wrong T8: markContainerForDelete(OLD) → DELETED Final state: OLD (Disk1): DELETED NEW (Disk2): CLOSED ← healthy, valid SCM view of container C: C on DN1 = UNHEALTHY ← wrong ``` **What SCM/RM does in response :** ``` T9: RM next cycle — ECUnderReplicationHandler.checkAndRemoveUnhealthyReplica() SCM replica record: DN1 has UNHEALTHY replica of C checks: is there a CLOSED replica for same index on another DN? → if YES: "prefer deleting the UNHEALTHY over CLOSED" → sendThrottledDeleteCommand(DN1) → if NO CLOSED elsewhere: "delete any UNHEALTHY" → sendThrottledDeleteCommand(DN1) T10: DN1 receives DeleteContainerCommand for C containerSet.getContainer(id) → NEW container (Disk2, CLOSED) NEW container DELETED ← healthy valid replica gone Now container C is genuinely under-replicated. RM tries to fix it by replicating — but the replica it just deleted was the source. ``` **Outcome:** A healthy replica on Disk2 gets deleted. Container C becomes genuinely under-replicated. The window between readUnlock (T7) and markContainerForDelete (T8) is the critical period. If the scanner's sendICR reaches SCM and RM processes it before FCR corrects the state, the delete command lands on DN1 and hits the healthy NEW container. This is why markContainerUnhealthy needs the re-fetch — the consequences of operating on the wrong container are irreversible. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4340923770 **Second Finding: For QUASI_CLOSED container if SCM sends force Close command and DiskBalancer is working on same container** **QUASI_CLOSED** containers can be force-closed by SCM (`CloseContainerCommand with force=true`). That goes through `controller.closeContainer()`. Here is the exact timeline: ``` DiskBalancer (DN1) CloseContainerCommandHandler ─ ─ T1: container = containerSet.getContainer(C) → OLD container (QUASI_CLOSED, Disk1) T2: container.readLock() on OLD T3: copy Disk1 → Disk2 ... T3a: container = containerSet.getContainer(C) → OLD container (before updateContainer) T3b: switch(container.getContainerState()) → QUASI_CLOSED + force=true → controller.closeContainer(id) → containerSet.getContainer(id) → OLD container (still, before T5) → container.close() → writeLock() → BLOCKED ←--- readLock held T4: copy done, atomic move to Disk2 T5: importContainer → newContainer (QUASI_CLOSED, Disk2) T6: containerSet.updateContainer(newContainer) ← ContainerSet now maps C → newContainer T7: container.readUnlock() ← releases OLD readLock T7a: writeLock ACQUIRED on OLD container → OLD: QUASI_CLOSED → CLOSED → sendICR(OLD=CLOSED) → SCM told C is CLOSED T8: container.markContainerForDelete(OLD) → writeLock → OLD: CLOSED → DELETED ``` **after T8** ``` State In ContainerSet? --- OLD container (Disk1) DELETED No (updateContainer removed it) NEW container (Disk2) QUASI_CLOSED Yes — this is the live replica ``` `SCM's view`: Container C on **DN1 = CLOSED** (from ICR sent at T7a), `Reality`: Container C on DN1 = **QUASI_CLOSED (newContainer)**. **This is a kind of regression:** SCM thinks it's **CLOSED**. But DN1's next container report says **QUASI_CLOSED**. SCM sees a state "regression" **(CLOSED → QUASI_CLOSED)**. Depending on the FCR sent to SCM, it may: Re-send a `force close command → controller.closeContainer(id)` now re-fetches from ContainerSet → gets NEW container → closes it correctly → CLOSED. Eventually converges. Or treat it as an unhealthy/inconsistent replica. No data loss — the data is intact on Disk2. But there is a state inconsistency window where SCM's cached state (CLOSED) differs from reality (QUASI_CLOSED on the new disk). I think here as well we need to re-fetch the container . -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4340781753 > This change looks good and I think it will solve the problem. However I think it reveals a problem with the code structure within the datanode. These services should not have to perform such complex locking to get a consistent view of a container and it would be very easy in the future for some other service to come along and not do things correctly, or indeed some other existing part of the DN code may also be doing things incorrectly already. > > I think that all the container "logic" should be hidden behind an interface and all the services like disk balancer, or block deletion should call it, eg: > > ``` > containerManager.moveContainer(container_id, destination) > containerManager.deleteBlockList(container_id, blocksToDelete) > ... > etc > ``` > > Then all the locking etc happens behind the scenes in the same place where it can be controlled more closely. > > Fixing this is a large exercise and not something we would want to take on in this PR. > > There are other places in KeyValueHandler that take the container lock - are we sure they are OK or is any change needed in them too? I agree with u @sodonnel that all locking mechanism should be hidden behind an interface that it happens at the same place. I was going through other write locks in the code base. I see that when **EC Under-replication + DiskBalancer** there can be failure however it cannot be the problem but we can minimise the effort here. **Setup:** Container C (RS-6-3), shard index=2 exists only on DN1 (under-replicated). DiskBalancer on DN1 simultaneously moves shard idx=2 from Disk1 → Disk2. ``` The race: DiskBalancer (DN1)EC Reconstruction (DN5 coordinator) ─ container.readLock() copy Disk1 → tmp/ pulls idx=0 from DN3 ✓ atomic move tmp → Disk2 pulls idx=1 from DN4 ✓ containerSet.updateContainer() containerSet.getContainer(C) → OLD obj (Disk1) readUnlock() tries writeLock → was blocked, now unblocked markContainerForDelete(OLD)OLD.state = DELETED exportContainerData checks state → DELETED throws IllegalStateException → FAILS ← entire ReconstructECContainersCommand fails bandwidth for idx=0, idx=1 already wasted ``` Net result: EC reconstruction fails just because the shard at DN1 has been marked as DELETED while it correctly pulled index from other DNs. Container C still has shard idx=2 missing. RM re-queues it next monitor cycle. Here we have two options: **Option 1 — Rely on RM to re-send (current behavior)** - RM re-queues the container after command failure/timeout - Next cycle: DN1's heartbeat has already reported the new path for idx=2 on Disk2 - RM sends a fresh ReconstructECContainersCommand — this time export succeeds on the NEW container - Cost: 1 extra monitor cycle delay + bandwidth for idx=0, idx=1 already wasted in the failed attempt - Safe? Yes — no data loss, just delayed fix. But every failed attempt wastes bandwidth for all N-1 other shards too. **Option 2 — Re-fetch container from ContainerSet after getting the lock like others** In `exportContainerData`, after acquiring the lock, re-fetch the container from ContainerSet by ID, so even if the old container is marked DELETED the new container moved by diskbalancer will be fetched and will not fail ECReconstruction. Please let me know which option do @sodonnel @ChenSammiyou both prefer ? RM retry is simpler and already works — it just costs one extra cycle and wasted network I/O for every time the race window is hit. I just wanted to bring this in your knowledge and discuss. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4336761146 This change looks good and I think it will solve the problem. However I think it reveals a problem with the code structure within the datanode. These services should not have to perform such complex locking to get a consistent view of a container and it would be very easy in the future for some other service to come along and not do things correctly, or indeed some other existing part of the DN code may also be doing things incorrectly already. I think that all the container "logic" should be hidden behind an interface and all the services like disk balancer, or block deletion should call it, eg: ``` containerManager.moveContainer(container_id, destination) containerManager.deleteBlockList(container_id, blocksToDelete) ... etc ``` Then all the locking etc happens behind the scenes in the same place where it can be controlled more closely. Fixing this is a large exercise and not something we would want to take on in this PR. There are other places in KeyValueHandler that take the container lock - are we sure they are OK or is any change needed in them too? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3153879538
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##
@@ -2296,6 +2296,15 @@ private void deleteInternal(Container container, boolean
force)
long startTime = clock.millis();
container.writeLock();
try {
+ // Re-fetch AFTER lock to verify the container is not having stale
references while waiting for lock.
+ long containerId = container.getContainerData().getContainerID();
+ Container current = containerSet.getContainer(containerId);
+ if (current == null || current != container) {
+LOG.warn("Container {} reference is stale. Aborting delete." +
+" SCM will retry with the current replica.", containerId);
+return;
Review Comment:
This retry is better rather than failing immediately. Thanks for updating 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3151966968
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##
@@ -484,23 +484,23 @@ public BackgroundTaskResult call() {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- // Double check container state before acquiring lock to start move
process.
- // Container state may have changed after selection. Only CLOSED
containers can be moved.
- // QUASI_CLOSED is allowed when test mode is enabled, this is done to
test in production
- // these containers are rejected.
- State containerState = container.getContainerData().getState();
- boolean isTestMode = DefaultContainerChoosingPolicy.isTest();
- if (containerState != State.CLOSED && !(isTestMode && containerState ==
State.QUASI_CLOSED)) {
-LOG.warn("Container {} is in {} state, skipping move process. Only
CLOSED containers can be moved.",
-containerId, containerState);
-postCall(false, startTime);
-return BackgroundTaskResult.EmptyTaskResult.newResult();
- }
-
// hold read lock on the container first, to avoid other threads to
update the container state,
// such as block deletion.
container.readLock();
try {
+// Double check container state after acquiring lock to start move
process.
+// Container state may have changed after selection. Only CLOSED
containers can be moved.
+// QUASI_CLOSED is allowed when test mode is enabled, this is done to
test in production
+// these containers are rejected.
+State containerState = container.getContainerData().getState();
+boolean isTestMode = DefaultContainerChoosingPolicy.isTest();
+if (containerState != State.CLOSED && !(isTestMode && containerState
== State.QUASI_CLOSED)) {
Review Comment:
Updated .
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3151964359
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##
@@ -2296,6 +2296,15 @@ private void deleteInternal(Container container, boolean
force)
long startTime = clock.millis();
container.writeLock();
try {
+ // Re-fetch AFTER lock to verify the container is not having stale
references while waiting for lock.
+ long containerId = container.getContainerData().getContainerID();
+ Container current = containerSet.getContainer(containerId);
+ if (current == null || current != container) {
+LOG.warn("Container {} reference is stale. Aborting delete." +
+" SCM will retry with the current replica.", containerId);
+return;
Review Comment:
Makes sense. Updated to retry logic. Please confirm if retry upto 10 times
is sufficient or need to decrease or increase 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3147305298
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##
@@ -2296,6 +2296,15 @@ private void deleteInternal(Container container, boolean
force)
long startTime = clock.millis();
container.writeLock();
try {
+ // Re-fetch AFTER lock to verify the container is not having stale
references while waiting for lock.
+ long containerId = container.getContainerData().getContainerID();
+ Container current = containerSet.getContainer(containerId);
+ if (current == null || current != container) {
+LOG.warn("Container {} reference is stale. Aborting delete." +
+" SCM will retry with the current replica.", containerId);
+return;
Review Comment:
RM will not resend the delete for 10 minutes, as it doesn't know if the task
has been queued or failed.
We shouldn't fail tasks for re-tryable reasons within the DN. If another
thread has the ability to modify the object between when we get it and we lock
it, then should we not always just get it again after locking and then we know
for sure nothing can have changed it, and proceed with the second copy? Why
even do a compare of first == second?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3146882314
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingTask.java:
##
@@ -139,24 +140,38 @@ public BackgroundTaskResult call() throws Exception {
private ContainerBackgroundTaskResult handleDeleteTask() throws Exception {
ContainerBackgroundTaskResult crr;
-final Container container = ozoneContainer.getContainerSet()
-.getContainer(containerData.getContainerID());
+ContainerSet cs = ozoneContainer.getContainerSet();
+final Container container =
cs.getContainer(containerData.getContainerID());
+if (container == null) {
+ return new ContainerBackgroundTaskResult();
+}
container.writeLock();
-File dataDir = new File(containerData.getChunksPath());
-long startTime = Time.monotonicNow();
-// Scan container's db and get list of under deletion blocks
-try (DBHandle meta = BlockUtils.getDB(containerData, conf)) {
- if (containerData.hasSchema(SCHEMA_V1)) {
-crr = deleteViaSchema1(meta, container, dataDir, startTime);
- } else if (containerData.hasSchema(SCHEMA_V2)) {
-crr = deleteViaSchema2(meta, container, dataDir, startTime);
- } else if (containerData.hasSchema(SCHEMA_V3)) {
-crr = deleteViaSchema3(meta, container, dataDir, startTime);
- } else {
-throw new UnsupportedOperationException(
-"Only schema version 1,2,3 are supported.");
+try {
+ // Re-fetch AFTER lock to verify the container is not having stale
references while waiting for lock.
+ Container current = cs.getContainer(containerData.getContainerID());
+ if (current == null || current.getContainerData() != containerData) {
+LOG.warn("Container {} reference is stale. Aborting delete. SCM will
retry with" +
+" the current replica.", containerData.getContainerID());
+return new ContainerBackgroundTaskResult();
Review Comment:
@sodonnel On second thought, I don't think an explicit retry loop is
necessary here. When this task returns early, `BlockDeletingService` or will
pick up the same container again in its next service interval **(default: 60 s,
configurable via ozone.block.deleting.service.interval)** and construct a fresh
BlockDeletingTask with the current ContainerData from ContainerSet — which at
that point will be the post-move replica. That is effectively the same retry,
without added complexity inside the task itself.
Adding an in-task retry would require re-fetching ContainerData, the pending
block list, schema version, and chunk paths from the new container —
essentially re-creating what the service already does automatically on its next
run. The only difference is a delay of at most one service interval, which is
acceptable for a background block-deletion task.
I will change the `LOG.warn` to `LOG.info` and add a note saying the
container will be picked up in the next service interval, to make the intent
clear.
Please do suggest if it makes sense or am I missing something.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3146891061
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##
@@ -2296,6 +2296,15 @@ private void deleteInternal(Container container, boolean
force)
long startTime = clock.millis();
container.writeLock();
try {
+ // Re-fetch AFTER lock to verify the container is not having stale
references while waiting for lock.
+ long containerId = container.getContainerData().getContainerID();
+ Container current = containerSet.getContainer(containerId);
+ if (current == null || current != container) {
+LOG.warn("Container {} reference is stale. Aborting delete." +
+" SCM will retry with the current replica.", containerId);
+return;
Review Comment:
Similarly for `KeyValueHandler.deleteInternal`: When the stale check fires
and the method returns without error, SCM does not receive an exception from
the DN. On DN's next heartbeat, it reports the container replica still present
(now at the new volume). SCM's `RatisOverReplicationHandler` (or
DeletingContainerHandler) sees the replica still exists and re-queues the
delete. ReplicationManager's over-replication queue is processed every **30 s
(hdds.scm.replication.over.replicated.interval),** so a fresh
`DeleteContainerCommand` is re-sent within that window, this time targeting the
correct, post-move Container object. Again, the retry is free. So simple return
is better than retyring inside the this delete task.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3146306053
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##
@@ -2296,6 +2296,15 @@ private void deleteInternal(Container container, boolean
force)
long startTime = clock.millis();
container.writeLock();
try {
+ // Re-fetch AFTER lock to verify the container is not having stale
references while waiting for lock.
+ long containerId = container.getContainerData().getContainerID();
+ Container current = containerSet.getContainer(containerId);
+ if (current == null || current != container) {
+LOG.warn("Container {} reference is stale. Aborting delete." +
+" SCM will retry with the current replica.", containerId);
+return;
Review Comment:
Also here, can we retry rather than just fail?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
sodonnel commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3146281529
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingTask.java:
##
@@ -139,24 +140,38 @@ public BackgroundTaskResult call() throws Exception {
private ContainerBackgroundTaskResult handleDeleteTask() throws Exception {
ContainerBackgroundTaskResult crr;
-final Container container = ozoneContainer.getContainerSet()
-.getContainer(containerData.getContainerID());
+ContainerSet cs = ozoneContainer.getContainerSet();
+final Container container =
cs.getContainer(containerData.getContainerID());
+if (container == null) {
+ return new ContainerBackgroundTaskResult();
+}
container.writeLock();
-File dataDir = new File(containerData.getChunksPath());
-long startTime = Time.monotonicNow();
-// Scan container's db and get list of under deletion blocks
-try (DBHandle meta = BlockUtils.getDB(containerData, conf)) {
- if (containerData.hasSchema(SCHEMA_V1)) {
-crr = deleteViaSchema1(meta, container, dataDir, startTime);
- } else if (containerData.hasSchema(SCHEMA_V2)) {
-crr = deleteViaSchema2(meta, container, dataDir, startTime);
- } else if (containerData.hasSchema(SCHEMA_V3)) {
-crr = deleteViaSchema3(meta, container, dataDir, startTime);
- } else {
-throw new UnsupportedOperationException(
-"Only schema version 1,2,3 are supported.");
+try {
+ // Re-fetch AFTER lock to verify the container is not having stale
references while waiting for lock.
+ Container current = cs.getContainer(containerData.getContainerID());
+ if (current == null || current.getContainerData() != containerData) {
+LOG.warn("Container {} reference is stale. Aborting delete. SCM will
retry with" +
+" the current replica.", containerData.getContainerID());
+return new ContainerBackgroundTaskResult();
Review Comment:
I don't think this should just fail the task. We can try again, perhaps
wrapping the logic in a retry loop, because it's highly likely it will work the
next time around.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4325566559 @sodonnel Please review this PR whenever you are free. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3145923918
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##
@@ -484,23 +484,23 @@ public BackgroundTaskResult call() {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- // Double check container state before acquiring lock to start move
process.
- // Container state may have changed after selection. Only CLOSED
containers can be moved.
- // QUASI_CLOSED is allowed when test mode is enabled, this is done to
test in production
- // these containers are rejected.
- State containerState = container.getContainerData().getState();
- boolean isTestMode = DefaultContainerChoosingPolicy.isTest();
- if (containerState != State.CLOSED && !(isTestMode && containerState ==
State.QUASI_CLOSED)) {
-LOG.warn("Container {} is in {} state, skipping move process. Only
CLOSED containers can be moved.",
-containerId, containerState);
-postCall(false, startTime);
-return BackgroundTaskResult.EmptyTaskResult.newResult();
- }
-
// hold read lock on the container first, to avoid other threads to
update the container state,
// such as block deletion.
container.readLock();
try {
+// Double check container state after acquiring lock to start move
process.
+// Container state may have changed after selection. Only CLOSED
containers can be moved.
+// QUASI_CLOSED is allowed when test mode is enabled, this is done to
test in production
+// these containers are rejected.
+State containerState = container.getContainerData().getState();
+boolean isTestMode = DefaultContainerChoosingPolicy.isTest();
+if (containerState != State.CLOSED && !(isTestMode && containerState
== State.QUASI_CLOSED)) {
Review Comment:
@ChenSammi, I thought to update this here as here we are testing the race
case. Anyway whichever of the two PR gets merged first I will change the other
Pr accordingly.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3145925995
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingTask.java:
##
@@ -139,24 +140,38 @@ public BackgroundTaskResult call() throws Exception {
private ContainerBackgroundTaskResult handleDeleteTask() throws Exception {
ContainerBackgroundTaskResult crr;
-final Container container = ozoneContainer.getContainerSet()
-.getContainer(containerData.getContainerID());
+ContainerSet cs = ozoneContainer.getContainerSet();
+final Container container =
cs.getContainer(containerData.getContainerID());
+if (container == null) {
+ return new ContainerBackgroundTaskResult();
+}
container.writeLock();
-File dataDir = new File(containerData.getChunksPath());
-long startTime = Time.monotonicNow();
-// Scan container's db and get list of under deletion blocks
-try (DBHandle meta = BlockUtils.getDB(containerData, conf)) {
- if (containerData.hasSchema(SCHEMA_V1)) {
-crr = deleteViaSchema1(meta, container, dataDir, startTime);
- } else if (containerData.hasSchema(SCHEMA_V2)) {
-crr = deleteViaSchema2(meta, container, dataDir, startTime);
- } else if (containerData.hasSchema(SCHEMA_V3)) {
-crr = deleteViaSchema3(meta, container, dataDir, startTime);
- } else {
-throw new UnsupportedOperationException(
-"Only schema version 1,2,3 are supported.");
+try {
+ // Re-fetch AFTER lock to verify the container is not having stale
references while waiting for lock.
+ Container current = cs.getContainer(containerData.getContainerID());
+ if (current == null || current.getContainerData() != containerData) {
+LOG.warn("Container {} reference is stale. Aborting delete. SCM will
retry with" +
Review Comment:
yaa missed typed 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3145923918
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##
@@ -484,23 +484,23 @@ public BackgroundTaskResult call() {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- // Double check container state before acquiring lock to start move
process.
- // Container state may have changed after selection. Only CLOSED
containers can be moved.
- // QUASI_CLOSED is allowed when test mode is enabled, this is done to
test in production
- // these containers are rejected.
- State containerState = container.getContainerData().getState();
- boolean isTestMode = DefaultContainerChoosingPolicy.isTest();
- if (containerState != State.CLOSED && !(isTestMode && containerState ==
State.QUASI_CLOSED)) {
-LOG.warn("Container {} is in {} state, skipping move process. Only
CLOSED containers can be moved.",
-containerId, containerState);
-postCall(false, startTime);
-return BackgroundTaskResult.EmptyTaskResult.newResult();
- }
-
// hold read lock on the container first, to avoid other threads to
update the container state,
// such as block deletion.
container.readLock();
try {
+// Double check container state after acquiring lock to start move
process.
+// Container state may have changed after selection. Only CLOSED
containers can be moved.
+// QUASI_CLOSED is allowed when test mode is enabled, this is done to
test in production
+// these containers are rejected.
+State containerState = container.getContainerData().getState();
+boolean isTestMode = DefaultContainerChoosingPolicy.isTest();
+if (containerState != State.CLOSED && !(isTestMode && containerState
== State.QUASI_CLOSED)) {
Review Comment:
@ChenSammi, I thought to update this here as here we are testing the race
case. Anyway whichever of the two PR gets merged first I will change it
according to that.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3145753653
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##
@@ -484,23 +484,23 @@ public BackgroundTaskResult call() {
return BackgroundTaskResult.EmptyTaskResult.newResult();
}
- // Double check container state before acquiring lock to start move
process.
- // Container state may have changed after selection. Only CLOSED
containers can be moved.
- // QUASI_CLOSED is allowed when test mode is enabled, this is done to
test in production
- // these containers are rejected.
- State containerState = container.getContainerData().getState();
- boolean isTestMode = DefaultContainerChoosingPolicy.isTest();
- if (containerState != State.CLOSED && !(isTestMode && containerState ==
State.QUASI_CLOSED)) {
-LOG.warn("Container {} is in {} state, skipping move process. Only
CLOSED containers can be moved.",
-containerId, containerState);
-postCall(false, startTime);
-return BackgroundTaskResult.EmptyTaskResult.newResult();
- }
-
// hold read lock on the container first, to avoid other threads to
update the container state,
// such as block deletion.
container.readLock();
try {
+// Double check container state after acquiring lock to start move
process.
+// Container state may have changed after selection. Only CLOSED
containers can be moved.
+// QUASI_CLOSED is allowed when test mode is enabled, this is done to
test in production
+// these containers are rejected.
+State containerState = container.getContainerData().getState();
+boolean isTestMode = DefaultContainerChoosingPolicy.isTest();
+if (containerState != State.CLOSED && !(isTestMode && containerState
== State.QUASI_CLOSED)) {
Review Comment:
@Gargi-jais11 , this is updated by HDDS-14926(#10022).
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3145730310
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingTask.java:
##
@@ -139,24 +140,38 @@ public BackgroundTaskResult call() throws Exception {
private ContainerBackgroundTaskResult handleDeleteTask() throws Exception {
ContainerBackgroundTaskResult crr;
-final Container container = ozoneContainer.getContainerSet()
-.getContainer(containerData.getContainerID());
+ContainerSet cs = ozoneContainer.getContainerSet();
+final Container container =
cs.getContainer(containerData.getContainerID());
+if (container == null) {
+ return new ContainerBackgroundTaskResult();
+}
container.writeLock();
-File dataDir = new File(containerData.getChunksPath());
-long startTime = Time.monotonicNow();
-// Scan container's db and get list of under deletion blocks
-try (DBHandle meta = BlockUtils.getDB(containerData, conf)) {
- if (containerData.hasSchema(SCHEMA_V1)) {
-crr = deleteViaSchema1(meta, container, dataDir, startTime);
- } else if (containerData.hasSchema(SCHEMA_V2)) {
-crr = deleteViaSchema2(meta, container, dataDir, startTime);
- } else if (containerData.hasSchema(SCHEMA_V3)) {
-crr = deleteViaSchema3(meta, container, dataDir, startTime);
- } else {
-throw new UnsupportedOperationException(
-"Only schema version 1,2,3 are supported.");
+try {
+ // Re-fetch AFTER lock to verify the container is not having stale
references while waiting for lock.
+ Container current = cs.getContainer(containerData.getContainerID());
+ if (current == null || current.getContainerData() != containerData) {
+LOG.warn("Container {} reference is stale. Aborting delete. SCM will
retry with" +
Review Comment:
Please change the log level to info, as it's an expected behavior and
doesn't need to alert user. Same for KeyValueHandler.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
ChenSammi commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3145694635
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/statemachine/background/BlockDeletingTask.java:
##
@@ -139,24 +140,38 @@ public BackgroundTaskResult call() throws Exception {
private ContainerBackgroundTaskResult handleDeleteTask() throws Exception {
ContainerBackgroundTaskResult crr;
-final Container container = ozoneContainer.getContainerSet()
-.getContainer(containerData.getContainerID());
+ContainerSet cs = ozoneContainer.getContainerSet();
+final Container container =
cs.getContainer(containerData.getContainerID());
+if (container == null) {
+ return new ContainerBackgroundTaskResult();
+}
container.writeLock();
-File dataDir = new File(containerData.getChunksPath());
-long startTime = Time.monotonicNow();
-// Scan container's db and get list of under deletion blocks
-try (DBHandle meta = BlockUtils.getDB(containerData, conf)) {
- if (containerData.hasSchema(SCHEMA_V1)) {
-crr = deleteViaSchema1(meta, container, dataDir, startTime);
- } else if (containerData.hasSchema(SCHEMA_V2)) {
-crr = deleteViaSchema2(meta, container, dataDir, startTime);
- } else if (containerData.hasSchema(SCHEMA_V3)) {
-crr = deleteViaSchema3(meta, container, dataDir, startTime);
- } else {
-throw new UnsupportedOperationException(
-"Only schema version 1,2,3 are supported.");
+try {
+ // Re-fetch AFTER lock to verify the container is not having stale
references while waiting for lock.
+ Container current = cs.getContainer(containerData.getContainerID());
+ if (current == null || current.getContainerData() != containerData) {
+LOG.warn("Container {} reference is stale. Aborting delete. SCM will
retry with" +
Review Comment:
Here is "DN retry", not "SCM will retry".
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
adoroszlai commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3130444723
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##
@@ -836,6 +834,18 @@ public static void setInjector(FaultInjector instance) {
injector = instance;
}
+ // call FaultInjector#pause when an injector is registered; ignore
IOException.
+ @VisibleForTesting
+ private static void pauseInjector() {
Review Comment:
nit: `@VisibleForTesting` is invalid on `private` method
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3129075037
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##
@@ -470,6 +470,7 @@ protected class DiskBalancerTask implements BackgroundTask {
}
@Override
+@SuppressWarnings("checkstyle:methodlength")
Review Comment:
Thanks @adoroszlai missed this part of extracting it to new method.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
adoroszlai commented on code in PR #10109:
URL: https://github.com/apache/ozone/pull/10109#discussion_r3128966156
##
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java:
##
@@ -470,6 +470,7 @@ protected class DiskBalancerTask implements BackgroundTask {
}
@Override
+@SuppressWarnings("checkstyle:methodlength")
Review Comment:
Please reduce method length instead of suppressing the warning. The
simplest way to achieve that, without mixing big refactoring and bugfix, is to
extract the injector `try-catch` block into a helper function.
https://github.com/apache/ozone/blob/2095f4f4c7192902aaa35ce6aec949d966b366d1/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java#L563-L569
https://github.com/apache/ozone/blob/2095f4f4c7192902aaa35ce6aec949d966b366d1/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/diskbalancer/DiskBalancerService.java#L587-L593
The method can be further refactored in follow-up.
##
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/diskbalancer/TestDiskBalancerWithConcurrentBackgroundTasks.java:
##
@@ -0,0 +1,457 @@
+/*
+ * 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.ozone.container.diskbalancer;
+
+import static
org.apache.hadoop.ozone.container.common.ContainerTestUtils.createDbInstancesForTestIfNeeded;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.io.File;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import org.apache.commons.io.FileUtils;
+import org.apache.hadoop.hdds.HddsConfigKeys;
+import org.apache.hadoop.hdds.conf.OzoneConfiguration;
+import org.apache.hadoop.hdds.fs.MockSpaceUsageCheckFactory;
+import org.apache.hadoop.hdds.fs.SpaceUsageCheckFactory;
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
+import
org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State;
+import org.apache.hadoop.hdds.scm.ScmConfigKeys;
+import org.apache.hadoop.hdds.utils.FaultInjector;
+import org.apache.hadoop.hdds.utils.db.Table;
+import org.apache.hadoop.ozone.container.checksum.ContainerChecksumTreeManager;
+import org.apache.hadoop.ozone.container.common.helpers.ContainerMetrics;
+import org.apache.hadoop.ozone.container.common.impl.BlockDeletingService;
+import org.apache.hadoop.ozone.container.common.impl.ContainerSet;
+import org.apache.hadoop.ozone.container.common.interfaces.Container;
+import org.apache.hadoop.ozone.container.common.interfaces.ContainerDispatcher;
+import org.apache.hadoop.ozone.container.common.interfaces.DBHandle;
+import org.apache.hadoop.ozone.container.common.interfaces.Handler;
+import
org.apache.hadoop.ozone.container.common.interfaces.VolumeChoosingPolicy;
+import org.apache.hadoop.ozone.container.common.volume.HddsVolume;
+import org.apache.hadoop.ozone.container.common.volume.MutableVolumeSet;
+import org.apache.hadoop.ozone.container.common.volume.StorageVolume;
+import org.apache.hadoop.ozone.container.common.volume.VolumeSet;
+import org.apache.hadoop.ozone.container.keyvalue.ContainerTestVersionInfo;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainer;
+import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData;
Re: [PR] HDDS-15066. Read-Write Lock race leave stale references to container creating orphan replicas [ozone]
Gargi-jais11 commented on PR #10109: URL: https://github.com/apache/ozone/pull/10109#issuecomment-4302265984 @ChenSammi @sadanand48 Please review the patch as per your convinence. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
