You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "ivandika3 (via GitHub)" <gi...@apache.org> on 2024/01/25 08:05:48 UTC

[PR] HDDS-10210. Ensure atomic update in StateContext#updateCommandStatus [ozone]

ivandika3 opened a new pull request, #6091:
URL: https://github.com/apache/ozone/pull/6091

   ## What changes were proposed in this pull request?
   
   While tracing the block deletion logic, I found a possible problem regarding command status update logic.
   
   The cmdStatusMap in StateContext is a ConcurrentHashMap
   
   Currently the update logic for command status is as follows:
   
   ```java
   public boolean updateCommandStatus(Long cmdId,
       Consumer<CommandStatus> cmdStatusUpdater) {
     if (cmdStatusMap.containsKey(cmdId)) {
       cmdStatusUpdater.accept(cmdStatusMap.get(cmdId));
       return true;
     }
     return false;
   }
   ```
   
   It does not seem correct. Although the get containsKey and get is protected by the ConcurrentHashMap, the CommandStatus retrieved is mutated directly using the cmdStatusUpdater and not protected by the ConcurrentHashMap (Please correct me if I'm mistaken). This might trigger race condition.
   
   We should use atomic operation like computeIfPresent.
   
   Note: The cmdStatusMap is also exposed to public (getCommandStatusMap) which might not be a good idea since we cannot guarantee correct usage of the map. We might need to encapsulate it properly inside StateContext.
   
   ## What is the link to the Apache JIRA
   
   https://issues.apache.org/jira/browse/HDDS-10210
   
   ## How was this patch tested?
   
   Existing tests.
   
   Clean CI: https://github.com/ivandika3/ozone/actions/runs/7650123708
   


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10210. Ensure atomic update in StateContext#updateCommandStatus [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6091:
URL: https://github.com/apache/ozone/pull/6091#issuecomment-1911909799

   Thanks @ivandika3 for the patch, @aswinshakil, @siddhantsangwan, @sodonnel for the review.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10210. Ensure atomic update in StateContext#updateCommandStatus [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai merged PR #6091:
URL: https://github.com/apache/ozone/pull/6091


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10210. Ensure atomic update in StateContext#updateCommandStatus [ozone]

Posted by "ivandika3 (via GitHub)" <gi...@apache.org>.
ivandika3 commented on PR #6091:
URL: https://github.com/apache/ozone/pull/6091#issuecomment-1911934811

   Thank you @aswinshakil, @siddhantsangwan, @sodonnel for the reviews and @adoroszlai for the merge.


-- 
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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org

Re: [PR] HDDS-10210. Ensure atomic update in StateContext#updateCommandStatus [ozone]

Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #6091:
URL: https://github.com/apache/ozone/pull/6091#issuecomment-1911590206

   @siddhantsangwan 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: issues-unsubscribe@ozone.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org