Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lease: use monotime in time.Time for Go 1.9 #8507

Merged
merged 1 commit into from
Sep 7, 2017
Merged

lease: use monotime in time.Time for Go 1.9 #8507

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 23 additions & 15 deletions lease/lessor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,18 @@ import (
"math"
"sort"
"sync"
"sync/atomic"
"time"

"github.com/coreos/etcd/lease/leasepb"
"github.com/coreos/etcd/mvcc/backend"
"github.com/coreos/etcd/pkg/monotime"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

git rm -r pkg/monotime too since this orphans the package

)

const (
// NoLease is a special LeaseID representing the absence of a lease.
NoLease = LeaseID(0)

forever = monotime.Time(math.MaxInt64)
)
// NoLease is a special LeaseID representing the absence of a lease.
const NoLease = LeaseID(0)

var (
forever = time.Time{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment here to explain why forever = time.Time{}.

Copy link
Contributor Author

@lorneli lorneli Sep 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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()."

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's too detailed. I am thinking about just say forever is represented by zero time.Time. Defer to @heyitsanthony for best comment.

Copy link
Contributor Author

@lorneli lorneli Sep 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please just use time.IsZero(); a separate type is excessive. The comment can be:

expiry time.Time // no expiration when expiry.IsZero() is true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.


leaseBucketName = []byte("lease")

// maximum number of leases to revoke per second; configurable for tests
Expand Down Expand Up @@ -560,8 +556,10 @@ func (le *lessor) initAndRecover() {
type Lease struct {
ID LeaseID
ttl int64 // time to live in seconds
// expiry is time when lease should expire; must be 64-bit aligned.
expiry monotime.Time
// expiryMu protects concurrent accesses to expiry
expiryMu sync.RWMutex
// expiry is time when lease should expire. no expiration when expiry.IsZero() is true
expiry time.Time

// mu protects concurrent accesses to itemSet
mu sync.RWMutex
Expand Down Expand Up @@ -594,12 +592,18 @@ func (l *Lease) TTL() int64 {

// refresh refreshes the expiry of the lease.
func (l *Lease) refresh(extend time.Duration) {
t := monotime.Now().Add(extend + time.Duration(l.ttl)*time.Second)
atomic.StoreUint64((*uint64)(&l.expiry), uint64(t))
newExpiry := time.Now().Add(extend + time.Duration(l.ttl)*time.Second)
l.expiryMu.Lock()
defer l.expiryMu.Unlock()
l.expiry = newExpiry
}

// forever sets the expiry of lease to be forever.
func (l *Lease) forever() { atomic.StoreUint64((*uint64)(&l.expiry), uint64(forever)) }
func (l *Lease) forever() {
l.expiryMu.Lock()
defer l.expiryMu.Unlock()
l.expiry = forever
}

// Keys returns all the keys attached to the lease.
func (l *Lease) Keys() []string {
Expand All @@ -614,8 +618,12 @@ func (l *Lease) Keys() []string {

// Remaining returns the remaining time of the lease.
func (l *Lease) Remaining() time.Duration {
t := monotime.Time(atomic.LoadUint64((*uint64)(&l.expiry)))
return time.Duration(t - monotime.Now())
l.expiryMu.RLock()
defer l.expiryMu.RUnlock()
if l.expiry.IsZero() {
return time.Duration(math.MaxInt64)
}
return l.expiry.Sub(time.Now())
}

type LeaseItem struct {
Expand Down
6 changes: 0 additions & 6 deletions pkg/monotime/issue15006.s
View file
Open in desktop

This file was deleted.

26 changes: 0 additions & 26 deletions pkg/monotime/monotime.go
View file
Open in desktop

This file was deleted.

24 changes: 0 additions & 24 deletions pkg/monotime/nanotime.go
View file
Open in desktop

This file was deleted.

22 changes: 0 additions & 22 deletions pkg/monotime/nanotime_test.go
View file
Open in desktop

This file was deleted.