Caideyipi commented on PR #17931: URL: https://github.com/apache/iotdb/pull/17931#issuecomment-4690024658
I found two issues that should be addressed before merging: 1. `SystemMetrics.java:65-66,337`: `volatile` does not make the mutable `HashSet`/`List` thread-safe. `collectDiskSpace()` iterates the current `fileStores` snapshot, but `removeSystemDiskInfo()` still mutates that same `HashSet` in place via `fileStores.clear()`. If an auto-gauge scrape or heartbeat is iterating while metrics are unbound, this can throw `ConcurrentModificationException`. This also matches the Sonar `java:S3077` findings on the new `volatile` collection fields. A safer copy-on-write shape would be to publish immutable snapshots, for example initialize with `Collections.emptySet()`/`Collections.emptyList()`, assign new unmodifiable copies in `setDiskDirs()` and `refreshFileStores()`, and replace `fileStores.clear()` with `this.fileStores = Collections.emptySet()`. 2. `SystemMetrics.java:374`: Sonar reports `if (refreshed)` as an always-false condition, and the current SonarCloud Code Analysis check is failing because of it. The runtime intent looks like two attempts: read once, refresh on failure, then read once more and warn for any remaining failures. I think it would be clearer and more analysis-friendly to express that directly, rather than using `while (true)` plus the `refreshed` flag. I verified the new test locally on the PR head with: `mvn -nsu -pl iotdb-core/metrics/interface -Dtest=SystemMetricsTest test` The test passed, including checkstyle and spotless in that Maven run. The remaining blocker I see is the SonarCloud failure above. -- 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]
