Skip to content

Commit

Permalink
Fix some obvious bugs in lease renewal
Browse files Browse the repository at this point in the history
  • Loading branch information
colinmarc committed Nov 27, 2019
1 parent 7eabc3a commit 1cf907d
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 13 deletions.
7 changes: 4 additions & 3 deletions file_writer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,20 +58,21 @@ func TestFileWrite(t *testing.T) {
assert.Equal(t, "foobar", string(bytes))
}

func TestFileWriteAfterIdleTime(t *testing.T) {
func TestFileWriteLeaseRenewal(t *testing.T) {
client := getClient(t)

baleet(t, "/_test/create/1.txt")
mkdirp(t, "/_test/create")

writer, err := client.Create("/_test/create/1.txt")
require.NoError(t, err)

n, err := writer.Write([]byte("foo"))
require.NoError(t, err)
assert.Equal(t, 3, n)

//Check that write after idle time doesn't fail
time.Sleep(90 * time.Second)
// Sleep long enough for the lease to expire.
time.Sleep(95 * time.Second)

n, err = writer.Write([]byte("bar"))
require.NoError(t, err)
Expand Down
25 changes: 15 additions & 10 deletions internal/rpc/namenode.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,27 +285,32 @@ func (c *NamenodeConnection) renewLeases() {
ticker := time.NewTicker(leaseRenewInterval)
defer ticker.Stop()

select {
case <-ticker.C:
req := &hdfs.RenewLeaseRequestProto{ClientName: proto.String(c.ClientName)}
resp := &hdfs.RenewLeaseResponseProto{}

// Ignore any errors.
c.Execute("renewLease", req, resp)
case <-c.done:
return
for {
select {
case <-ticker.C:
req := &hdfs.RenewLeaseRequestProto{ClientName: proto.String(c.ClientName)}
resp := &hdfs.RenewLeaseResponseProto{}

// Ignore any errors.
c.Execute("renewLease", req, resp)
case <-c.done:
return
}
}
}

// Close terminates all underlying socket connections to remote server.
func (c *NamenodeConnection) Close() error {
close(c.done)

This comment has been minimized.

Copy link
@alexryndin

alexryndin Nov 27, 2019

Thanks for fixing. This part still looks weird for me. Why instead of sending done signal like that:done<-struct{}{}, you just close channel? This way renewLeases() will never reach return in for loop (Look here https://play.golang.org/p/PrF-l1sCWVy). Also this code looks more error prone and possibly contains race conditions (as pointed here 574b0ba#r36128585). We merged leaseRenew() realization by @efirs and that's been working pretty fine for a couple of month in production.

This comment has been minimized.

Copy link
@colinmarc

colinmarc Nov 27, 2019

Author Owner

Closing a channel works too. Your example is exiting before it prints.

This comment has been minimized.

Copy link
@alexryndin

alexryndin Nov 27, 2019

oh, thanks for pointing me that :) !


// Ensure that we're not concurrently renewing leases.
c.reqLock.Lock()
defer c.reqLock.Unlock()

if c.conn != nil {
return c.conn.Close()
}

close(c.done)
return nil
}

Expand Down

0 comments on commit 1cf907d

Please sign in to comment.