[util] Fix an issue with xtime.UnixNano.Truncate #3677

arnikola · 2021-08-20T16:56:37Z

This was previously not correctly following truncation as in time.Time,
since time.Time truncation calculates truncation duration from the date
January 1, year 1, 00:00:00.000000000 UTC, rather than epoch.

This was previously not correctly following truncation as in time.Time, since time.Time truncation calculates truncation duration from the date January 1, year 1, 00:00:00.000000000 UTC, rather than epoch.

ryanhall07 · 2021-08-20T17:01:00Z

src/x/time/unix_nano.go

@@ -94,12 +124,13 @@ func (u UnixNano) IsZero() bool {
 // String returns the time formatted using the format string
 //	"2006-01-02 15:04:05.999999999 -0700 MST"
 func (u UnixNano) String() string {
+	// return fmt.Sprint(u.ToTime().UTC().UnixNano())


ryanhall07 · 2021-08-20T17:01:35Z

src/x/time/time.go

@@ -31,7 +31,7 @@ const (

 // Now returns the current local time.


is this comment still correct? doesn't seem to be local time anymore

Oops yeah the comment is correct, should have reverted this change

I think the comment is not correct, either. There is no notion of time zone on unix timestamp.
This ToUnixNano uses go time.UnixNano() under the hood, and see the end of it's comment:

// UnixNano returns t as a Unix time, the number of nanoseconds elapsed // since January 1, 1970 UTC. The result is undefined if the Unix time // in nanoseconds cannot be represented by an int64 (a date before the year // 1678 or after 2262). Note that this means the result of calling UnixNano // on the zero Time is undefined. The result does not depend on the // location associated with t. func (t Time) UnixNano() int64 { return (t.unixSec())*1e9 + int64(t.nsec()) }

So I would change our comment to something more formally correct like "// Now returns the current Unix time."

ryanhall07 · 2021-08-20T17:02:30Z

src/x/time/unix_nano.go

+	// NB:Number of seconds from epoch to timeZero.
+	unixToInternal int64 = (1969*365 + 1969/4 - 1969/100 + 1969/400) * secondsPerDay
+)
+
 // Truncate returns the result of rounding u down to a multiple of d.
 // If d <= 0, Truncate returns u unchanged.


update comment to be d <= 1

codecov · 2021-08-20T17:46:42Z

Codecov Report

Merging #3677 (0df1e39) into master (0df1e39) will not change coverage.
The diff coverage is n/a.

❗ Current head 0df1e39 differs from pull request most recent head baf4a31. Consider uploading reports for the commit baf4a31 to get more accurate results

@@          Coverage Diff           @@
##           master   #3677   +/-   ##
======================================
  Coverage    56.6%   56.6%           
======================================
  Files         551     551           
  Lines       61909   61909           
======================================
  Hits        35089   35089           
  Misses      23664   23664           
  Partials     3156    3156

Flag	Coverage Δ
aggregator	`61.4% <0.0%> (ø)`
cluster	`∅ <0.0%> (∅)`
collector	`58.4% <0.0%> (ø)`
dbnode	`60.6% <0.0%> (ø)`
m3em	`46.4% <0.0%> (ø)`
metrics	`19.7% <0.0%> (ø)`
msg	`74.2% <0.0%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0df1e39...baf4a31. Read the comment docs.

robskillington · 2021-08-20T20:17:35Z

src/x/time/time_test.go

+
+func TestTruncate(t *testing.T) {
+	var (
+		n      = time.Now().UTC()


nit: Perhaps best to set this to a static date? I wonder if it would flake if it's always "now".

Also maybe a few different "nows" should be tested that they all truncate properly, i.e. run tests on a few of them against all block sizes.

Do we have any way of marking a flake as not a flake 😛 ? If this flakes then it implies there's still some underlying problem with truncation, would be useful to fail out if at all possible

linasm

LGTM just suggest fixing one existing comment to reduce confusion.

linasm · 2021-08-23T13:03:46Z

src/x/time/unix_nano.go

@@ -99,7 +129,7 @@ func (u UnixNano) String() string {

 // Format returns the string representation for the time with the given format.
 func (u UnixNano) Format(blockTimeFormat string) string {
-	return u.ToTime().Format(blockTimeFormat)
+	return u.ToTime().UTC().Format(blockTimeFormat)


Shouldn't .UTC() conversion happen inside u.ToTime()?

Good call 👍

@arnikola what was the reason to move .UTC() out of the UnixNano.ToTime() again before the merge?

It was causing problems with failing tests elsewhere; I'll put out a followup that will since they merit further investigation to double check it's the tests that need updating rather than the code

linasm · 2021-08-23T13:26:43Z

src/x/time/time.go

@@ -31,7 +31,7 @@ const (

 // Now returns the current local time.


I think the comment is not correct, either. There is no notion of time zone on unix timestamp.
This ToUnixNano uses go time.UnixNano() under the hood, and see the end of it's comment:

// UnixNano returns t as a Unix time, the number of nanoseconds elapsed // since January 1, 1970 UTC. The result is undefined if the Unix time // in nanoseconds cannot be represented by an int64 (a date before the year // 1678 or after 2262). Note that this means the result of calling UnixNano // on the zero Time is undefined. The result does not depend on the // location associated with t. func (t Time) UnixNano() int64 { return (t.unixSec())*1e9 + int64(t.nsec()) }

So I would change our comment to something more formally correct like "// Now returns the current Unix time."

linasm

LGTM

linasm · 2021-08-24T08:14:41Z

@arnikola can we land this?

…kola/time-truncate

[util] Fix an issue with xtime.UnixNano.Truncate

066611a

This was previously not correctly following truncation as in time.Time, since time.Time truncation calculates truncation duration from the date January 1, year 1, 00:00:00.000000000 UTC, rather than epoch.

ryanhall07 reviewed Aug 20, 2021

View reviewed changes

ryanhall07 approved these changes Aug 20, 2021

View reviewed changes

Lint + PR

d2b3af1

arnikola requested a review from linasm August 20, 2021 17:33

robskillington reviewed Aug 20, 2021

View reviewed changes

linasm approved these changes Aug 23, 2021

View reviewed changes

arnikola added 2 commits August 23, 2021 19:56

PR response

d7b71c1

Merge branch 'master' into arnikola/time-truncate

fed0106

linasm approved these changes Aug 23, 2021

View reviewed changes

arnikola added 3 commits August 24, 2021 17:31

UTC in string format

6235cb2

Merge branch 'master' into arnikola/time-truncate

dd8f304

Merge branch 'arnikola/time-truncate' of github.com:m3db/m3 into arni…

baf4a31

…kola/time-truncate

arnikola merged commit fbb98ba into master Aug 24, 2021

arnikola deleted the arnikola/time-truncate branch August 24, 2021 14:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[util] Fix an issue with xtime.UnixNano.Truncate #3677

[util] Fix an issue with xtime.UnixNano.Truncate #3677

arnikola commented Aug 20, 2021

ryanhall07 Aug 20, 2021

ryanhall07 Aug 20, 2021

arnikola Aug 20, 2021

linasm Aug 23, 2021

ryanhall07 Aug 20, 2021

codecov bot commented Aug 20, 2021 •

edited

Loading

robskillington Aug 20, 2021 •

edited

Loading

arnikola Aug 23, 2021

linasm left a comment

linasm Aug 23, 2021

arnikola Aug 23, 2021

linasm Aug 25, 2021

arnikola Aug 25, 2021

linasm Aug 23, 2021

linasm left a comment

linasm commented Aug 24, 2021

		@@ -31,7 +31,7 @@ const (

		// Now returns the current local time.

[util] Fix an issue with xtime.UnixNano.Truncate #3677

[util] Fix an issue with xtime.UnixNano.Truncate #3677

Conversation

arnikola commented Aug 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 20, 2021 • edited Loading

Codecov Report

robskillington Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linasm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linasm left a comment

Choose a reason for hiding this comment

linasm commented Aug 24, 2021

codecov bot commented Aug 20, 2021 •

edited

Loading

robskillington Aug 20, 2021 •

edited

Loading