SteNicholas opened a new pull request, #3150: URL: https://github.com/apache/celeborn/pull/3150
### What changes were proposed in this pull request? Remove the redundant synchronized keyword from the `isTerminated` method in `sameThreadExecutorService()` implementation within the `ThreadUtils` class. The method was using both a synchronized block and an explicit `ReentrantLock`, which is unnecessary and potentially problematic. Backport https://github.com/apache/spark/pull/50210. ### Why are the changes needed? The changes are needed for several reasons: 1. **Eliminates redundant synchronization**: The current implementation uses both synchronized keyword and explicit ReentrantLock, which is redundant and creates unnecessary overhead. 2. **Reduces potential deadlock risks**: Using two different locking mechanisms (built-in synchronized and explicit `ReentrantLock`) in the same method could lead to complex lock ordering issues and increase deadlock risks. Although the risk of deadlock in the current implementation is low, if someone modifies the code in the future and adds a method that acquires these two locks in a different order, it would introduce a deadlock risk. 3. **Improves performance**: Removing the unnecessary synchronization layer reduces lock contention and context switching overhead. 4. **Code consistency**: Other methods in the same class only use `ReentrantLock` for synchronization, so removing synchronized makes the code style more consistent. 5. **More precise control**: `ReentrantLock` already provides all the synchronization needed with more features than the implicit synchronized keyword. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? CI. -- 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]
