chia7712 commented on code in PR #22454:
URL: https://github.com/apache/kafka/pull/22454#discussion_r3411315422
##########
server/src/main/java/org/apache/kafka/server/metrics/LinuxIoMetricsCollector.java:
##########
@@ -79,32 +141,51 @@ public long writeBytes() {
* read_bytes: 0
* write_bytes: 0
* cancelled_write_bytes: 0
+ *
+ * <p>Caller must hold the monitor of this instance. {@code lastUpdateMs}
is written last so
+ * that any thread observing the new timestamp via a volatile read also
observes all updated
+ * cached fields.
*/
private boolean updateValues(long now) {
- synchronized (this) {
- try {
- cachedReadBytes = -1L;
- cachedWriteBytes = -1L;
- List<String> lines = Files.readAllLines(path,
StandardCharsets.UTF_8);
- for (String line : lines) {
- if (line.startsWith(READ_BYTES_PREFIX)) {
- cachedReadBytes =
Long.parseLong(line.substring(READ_BYTES_PREFIX.length()));
- } else if (line.startsWith(WRITE_BYTES_PREFIX)) {
- cachedWriteBytes =
Long.parseLong(line.substring(WRITE_BYTES_PREFIX.length()));
- }
+ try {
+ cachedReadBytes = -1L;
Review Comment:
If you update them with temporary negative values, other concurrent calls
taking the lock-free path will see this `-1L` state, right? It might be safer
to use local variables for parsing and update the volatile fields all at once
upon success
--
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]
