rkhachatryan commented on a change in pull request #17757:
URL: https://github.com/apache/flink/pull/17757#discussion_r754208421



##########
File path: 
flink-dstl/flink-dstl-dfs/src/main/java/org/apache/flink/changelog/fs/StateChangeFsUploader.java
##########
@@ -92,6 +103,16 @@ public void upload(Collection<UploadTask> tasks) throws 
IOException {
         }
     }
 
+    private LocalResult uploadWithMetrics(Path path, Collection<UploadTask> 
tasks)
+            throws IOException {
+        metrics.getUploadsCounter().inc();
+        long start = clock.relativeTimeNanos();
+        LocalResult result = upload(path, tasks);
+        metrics.getUploadLatencies().update(clock.relativeTimeNanos() - start);

Review comment:
       Is your question about a) `System.currentTimeMillis()` vs 
`System.nanoTime()` or b) whether nanos should be converted to milllis?
   As for a), I think `currentTimeMillis` should NOT be used as it's not 
monotonic (I consider potential overhead of `nanoTime` is neglibible in context 
of IO)
   As for b), I don't have a strong opinion. Nanosecond precision is consistent 
with State Backends latency tracking and potentially with future changelog 
implementations; it can also be useful in local setups (for debugging).
   Division can be done closer to UI then if needed.




-- 
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]


Reply via email to