[GitHub] [skywalking] kezhenxu94 commented on a change in pull request #5585: fix timebucket converter issue

[GitBox <gi...@apache.org>]

kezhenxu94 commented on a change in pull request #5585:
URL: https://github.com/apache/skywalking/pull/5585#discussion_r496679805



##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/analysis/TimeBucket.java
##########
@@ -104,6 +104,10 @@ public static boolean isDayBucket(long timeBucket) {
     public static long getTimestamp(long timeBucket, DownSampling downsampling) {
         Calendar calendar = Calendar.getInstance();
         calendar.setTimeInMillis(0);
+        calendar.set(Calendar.MILLISECOND, 0);
+        calendar.set(Calendar.SECOND, 0);
+        calendar.set(Calendar.MINUTE, 0);
+        calendar.set(Calendar.HOUR_OF_DAY, 0);

Review comment:
       Do you need these if you already have `calendar.setTimeInMillis(0);`? 

##########
File path: oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/analysis/TimeBucketTest.java
##########
@@ -18,63 +18,54 @@
 
 package org.apache.skywalking.oap.server.core.analysis;
 
-import java.util.concurrent.TimeUnit;
 import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 
-import static java.util.concurrent.TimeUnit.DAYS;
-import static java.util.concurrent.TimeUnit.HOURS;
-import static java.util.concurrent.TimeUnit.MILLISECONDS;
-import static java.util.concurrent.TimeUnit.MINUTES;
-import static java.util.concurrent.TimeUnit.SECONDS;
+import java.util.Calendar;
+import java.util.TimeZone;
 
 @RunWith(Parameterized.class)
 public class TimeBucketTest {
     private static final long NOW = System.currentTimeMillis();
 
     @Parameterized.Parameters
-    public static Object[][] parameters() {
-        return new Object[][] {
-            {
+    public static Object[] parameters() {
+        return new Object[]{
                 DownSampling.Second,
-                SECONDS,
-                MILLISECONDS.toSeconds(NOW)
-            },
-            {
                 DownSampling.Minute,
-                MINUTES,
-                MILLISECONDS.toMinutes(NOW)
-            },
-            {
                 DownSampling.Hour,
-                HOURS,
-                MILLISECONDS.toHours(NOW)
-            },
-            {
-                DownSampling.Day,
-                DAYS,
-                MILLISECONDS.toDays(NOW)
-            }
+                DownSampling.Day
         };
     }
 
     private DownSampling downSampling;
-    private TimeUnit unit;
-    private long time;
 
-    public TimeBucketTest(DownSampling downSampling, TimeUnit unit, long time) {
+    public TimeBucketTest(DownSampling downSampling) {
         this.downSampling = downSampling;
-        this.unit = unit;
-        this.time = time;
     }
 
     @Test
     public void testConversion() {
-        long timestamp = TimeBucket
-            .getTimestamp(TimeBucket.getTimeBucket(NOW, downSampling));
-        Assert.assertEquals(timestamp, unit.toMillis(time));
-    }
+        long timestamp = TimeBucket.getTimestamp(TimeBucket.getTimeBucket(NOW, downSampling));
 
+        Calendar instance = Calendar.getInstance(TimeZone.getDefault());
+        instance.setTimeInMillis(NOW);
+        switch (downSampling) {
+            case Day: {
+                instance.set(Calendar.HOUR_OF_DAY, 0);
+            }
+            case Hour: {
+                instance.set(Calendar.MINUTE, 0);
+            }
+            case Minute: {
+                instance.set(Calendar.SECOND, 0);
+            }
+            case Second: {
+                instance.set(Calendar.MILLISECOND, 0);
+            }

Review comment:
       Suggest to add comment about the deliberate "fall through" of the `switch` statement (as well as the ordering), in case future developers consider it as a bug, which is common




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

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