shameersss1 commented on PR #7828: URL: https://github.com/apache/hadoop/pull/7828#issuecomment-3117164023
Thanks @sjlee for the review. Please find the answers inline > * Why are we using the read lock for a mutation operation? Shouldn't we be using the write lock? The read lock will still permit concurrent operation and is not the right thing to use here, no? The method `refreshNodeAttributesToScheduler` does not do any writing. It only reads the variable host.attributes which can potentially be modified by some other thread leading to concurrent modification exception. We are also creating defensive copy so that further access is safe. ReadLock ensures that `refreshNodeAttributesToScheduler` can be accessed by multiple threads (since there is not writing) and the critical block `newNodeToAttributesMap.put(hostName, new HashSet<>(host.attributes.keySet()));` is protected. > * Regarding the unit test, I wonder how it is passing even with the read lock? Maybe the concurrency is not enough to reproduce the problem? It would be great if you could reproduce the problem with the old code first and prove that the new code fixes it. Since it is raise condition - Replication through unit test is difficult without inducing artificial sleeps in the core code flow. Ye, the unit test passes even without this change as well. The purpose of this unit test is more of protective measure. > * Have you done a fully analysis of the all reads and writes to this hashmap so that all read access is protected by the read lock and all write access by the write lock? That is the correct thing to do here. As per my analysis `host.attributes` is accessed during node attribute add , removal and replacing - every access except this one is protected using read/write 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: common-issues-unsubscr...@hadoop.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: common-issues-h...@hadoop.apache.org
