Dzeri96 commented on code in PR #3597:
URL: https://github.com/apache/celeborn/pull/3597#discussion_r2787039684
##########
worker/src/main/scala/org/apache/celeborn/service/deploy/worker/Worker.scala:
##########
@@ -284,11 +284,10 @@ private[celeborn] class Worker(
storageManager.updateDiskInfos()
storageManager.startDeviceMonitor()
- // WorkerInfo's diskInfos is a reference to storageManager.diskInfos
- val diskInfos = JavaUtils.newConcurrentHashMap[String, DiskInfo]()
- storageManager.disksSnapshot().foreach { diskInfo =>
- diskInfos.put(diskInfo.mountPoint, diskInfo)
- }
+ private val diskInfos = storageManager
+ .allDisksSnapshot()
+ .map { diskInfo => diskInfo.mountPoint -> diskInfo }
+ .toMap.asJava
Review Comment:
See... this is straight up wrong. In WorkerInfo's constructor, the argument
is copied into a `ConcurrentHashMap` before being assigned to a local member:
```scala
val diskInfos = {
if (_diskInfos != null) JavaUtils.newConcurrentHashMap[String,
DiskInfo](_diskInfos)
else null
}
```
I agree that AI can spot some issues that are impossible for compilers, and
very hard for humans to catch, but half the time you spend arguing with it over
obviously wrong and easily-verifiable statements. I think it's much better for
a human to take an hour to review this, than to spend 3 hours arguing with an
AI.
--
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]
