lease: use monotime in time.Time for Go 1.9 #8507
Conversation
|
This is a draft. I have several confusions when implementing this commit: a. It's hard to find an elegant way to express b. |
Codecov Report
@@ Coverage Diff @@
## master #8507 +/- ##
=========================================
Coverage ? 75.46%
=========================================
Files ? 356
Lines ? 29101
Branches ? 0
=========================================
Hits ? 21960
Misses ? 5588
Partials ? 1553
Continue to review full report at Codecov.
|
lease/lessor.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| forever = time.Unix(1<<63-1-unixTimeSeconds, maxNanoseconds) |
There was a problem hiding this comment.
We shouldn't use time.Unix because the time returned by time.Unix won't include monotonic information as described in
time.Unix, as well as the unmarshalers t.GobDecode, t.UnmarshalBinary. t.UnmarshalJSON, and t.UnmarshalText always create times with no monotonic clock reading.
There was a problem hiding this comment.
@fanminshi You're right. Since forever variable returned by time.Unix doesn't include monotonic information, time.Sub used in Lease.Remaining calculates in wall time, which may cause an unreasonable phenomenon: remaining time increases itself.
There was a problem hiding this comment.
@lorneli @heyitsanthony's suggestion using time.Time{} as forever sounds like a good idea.
There was a problem hiding this comment.
@fanminshi I think so too. Changing my commit with @heyitsanthony 's suggestion.
| "time" | ||
|
|
||
| "github.com/coreos/etcd/lease/leasepb" | ||
| "github.com/coreos/etcd/mvcc/backend" | ||
| "github.com/coreos/etcd/pkg/monotime" |
There was a problem hiding this comment.
git rm -r pkg/monotime too since this orphans the package
lease/lessor.go
Outdated
| return time.Duration(t - monotime.Now()) | ||
| l.expiryMu.RLock() | ||
| defer l.expiryMu.RUnlock() | ||
| return l.expiry.Sub(time.Now()) |
There was a problem hiding this comment.
since forever breaks down when switching to time.Time, this patch could instead use time.Time{} to mean forever:
func (l *Lease) Remaining() time.Duration {
l.expiryMu.RLock()
defer l.expiryMu.RUnlock()
if l.expiry.IsZero() {
return time.Duration(math.MaxInt64)
}
return time.Until(l.expiry)
}
lorneli
force-pushed
the
lease_monotime
branch
from
4907186
to
d56e0f9
Compare
|
@heyitsanthony Thanks for your suggestions. I've changed this commit: Still use |
|
|
||
| var ( | ||
| forever = time.Time{} |
There was a problem hiding this comment.
add a comment here to explain why forever = time.Time{}.
There was a problem hiding this comment.
@fanminshi How about this?
"Since time.Time with monotonic time only returned by time.Now(), it's hard to create a max time.Time variable with monotonic time. Instead, use zero time.Time to mean forever. It's easy to find whether a expiry is forever via Time.IsZero()."
There was a problem hiding this comment.
that's too detailed. I am thinking about just say forever is represented by zero time.Time. Defer to @heyitsanthony for best comment.
There was a problem hiding this comment.
Maybe use an explicit flag isForever, instead of a zero time.Time variable?
type expiry struct{
time.Time
isForever bool // set when lessor is demoted
}
func (l *Lease) Remaining() time.Duration {
l.expiryMu.RLock()
defer l.expiryMu.RUnlock()
if l.expiry.isForever {
return time.Duration(math.MaxInt64)
}
return l.expiry.Sub(time.Now())
}
There was a problem hiding this comment.
Please just use time.IsZero(); a separate type is excessive. The comment can be:
expiry time.Time // no expiration when expiry.IsZero() is true
The golang/time package tracks monotonic time in each time.Time returned by time.Now function for Go 1.9. Use time.Time to measure whether a lease is expired and remove previous pkg/monotime. Use zero time.Time to mean forever. No expiration when expiry.IsZero() is true.
lorneli
force-pushed
the
lease_monotime
branch
from
d56e0f9
to
63aa64d
Compare
|
Update comment 😀 |
|
Semaphore fails at compactor. |
|
lgtm. |
The golang/time package tracks monotonic time in each time.Time
returned by time.Now function for Go 1.9.
Use time.Time to measure whether a lease is expired, instead of previous
pkg/monotime.