[GitHub] [kafka] divijvaidya commented on a diff in pull request #13738: KAFKA-14982: Improve the kafka-metadata-quorum output

["divijvaidya (via GitHub)" <gi...@apache.org>]

divijvaidya commented on code in PR #13738:
URL: https://github.com/apache/kafka/pull/13738#discussion_r1199800198


##########
tools/src/main/java/org/apache/kafka/tools/MetadataQuorumCommand.java:
##########
@@ -155,17 +163,28 @@ private static void handleDescribeReplication(Admin admin) throws ExecutionExcep
         );
     }
 
-    private static List<List<String>> quorumInfoToRows(QuorumInfo.ReplicaState leader, Stream<QuorumInfo.ReplicaState> infos, String status) {
-        return infos.map(info ->
-            Stream.of(
+    private static List<List<String>> quorumInfoToRows(QuorumInfo.ReplicaState leader,
+                                                       Stream<QuorumInfo.ReplicaState> infos,
+                                                       String status,
+                                                       boolean readable) {
+        return infos.map(info -> {
+            final double nowMs = System.currentTimeMillis();

Review Comment:
   Consider using `Time.SYSTEM`



##########
tools/src/test/java/org/apache/kafka/tools/MetadataQuorumCommandTest.java:
##########
@@ -172,4 +173,22 @@ public void testDescribeQuorumInZkMode() {
         );
 
     }
+
+    @ClusterTests({
+        @ClusterTest(clusterType = Type.CO_KRAFT, brokers = 1, controllers = 1),
+    })
+    public void testHumanReadableTimestamps() {

Review Comment:
   this test doesn't test the business logic that we introduced in the code, i.e. the part about ensuring X in "X ms ago" is what is desired is missing.



##########
tools/src/main/java/org/apache/kafka/tools/MetadataQuorumCommand.java:
##########
@@ -155,17 +163,28 @@ private static void handleDescribeReplication(Admin admin) throws ExecutionExcep
         );
     }
 
-    private static List<List<String>> quorumInfoToRows(QuorumInfo.ReplicaState leader, Stream<QuorumInfo.ReplicaState> infos, String status) {
-        return infos.map(info ->
-            Stream.of(
+    private static List<List<String>> quorumInfoToRows(QuorumInfo.ReplicaState leader,
+                                                       Stream<QuorumInfo.ReplicaState> infos,
+                                                       String status,
+                                                       boolean readable) {
+        return infos.map(info -> {
+            final double nowMs = System.currentTimeMillis();
+            String lastFetchTimestamp = !info.lastFetchTimestamp().isPresent() ? "-1" :
+                valueOf(readable ? format("%.0f ms ago", nowMs - info.lastFetchTimestamp().getAsLong())

Review Comment:
   please expect skew/drift in system clock. For such situations we should handle the case when difference is negative.



##########
tools/src/main/java/org/apache/kafka/tools/MetadataQuorumCommand.java:
##########
@@ -79,6 +82,10 @@ static void execute(String... args) throws Exception {
             .addArgument("--command-config")
             .type(Arguments.fileType())
             .help("Property file containing configs to be passed to Admin Client.");
+        parser

Review Comment:
   this should probably go inside `addDescribeParser` since we want to parse for this only when --describe is called



##########
tools/src/main/java/org/apache/kafka/tools/MetadataQuorumCommand.java:
##########
@@ -92,11 +99,12 @@ static void execute(String... args) throws Exception {
             props.put(AdminClientConfig.BOOTSTRAP_SERVERS_CONFIG, namespace.getString("bootstrap_server"));
             admin = Admin.create(props);
 
+            boolean readable = Optional.of(namespace.getBoolean("human_readable")).orElse(false);

Review Comment:
   we only use this for --describe and replication, please move inside `handleDescribeReplication()`



##########
tools/src/main/java/org/apache/kafka/tools/MetadataQuorumCommand.java:
##########
@@ -155,17 +163,28 @@ private static void handleDescribeReplication(Admin admin) throws ExecutionExcep
         );
     }
 
-    private static List<List<String>> quorumInfoToRows(QuorumInfo.ReplicaState leader, Stream<QuorumInfo.ReplicaState> infos, String status) {
-        return infos.map(info ->
-            Stream.of(
+    private static List<List<String>> quorumInfoToRows(QuorumInfo.ReplicaState leader,
+                                                       Stream<QuorumInfo.ReplicaState> infos,
+                                                       String status,
+                                                       boolean readable) {
+        return infos.map(info -> {
+            final double nowMs = System.currentTimeMillis();

Review Comment:
   `System.currentTimeMillis()` returns a long. https://docs.oracle.com/javase/8/docs/api/java/lang/System.html#currentTimeMillis
   
   Using long return type here will makes subtraction easier later on in the code.



-- 
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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org