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.

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

api: lease list #8358

Merged
merged 9 commits into from
Aug 14, 2017
Merged

api: lease list #8358

merged 9 commits into from
Aug 14, 2017

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Aug 2, 2017

@heyitsanthony Should we return remaining on each lease, like TimeToLive?

@gyuho gyuho added the WIP label Aug 2, 2017
@heyitsanthony
Copy link
Contributor

@gyuho probably better to keep it simple for now; can add TTLs later if there's a need

@gyuho gyuho changed the title [DO NOT MERGE] *: add LeaseList RPC api: list leases Aug 8, 2017
@gyuho gyuho changed the title api: list leases api: lease list Aug 8, 2017
@gyuho gyuho removed the WIP label Aug 9, 2017
Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

mostly concerned about naming this LeaseList and how it gets called List in the client

@@ -98,6 +110,9 @@ type Lease interface {
// TimeToLive retrieves the lease information of the given lease ID.
TimeToLive(ctx context.Context, id LeaseID, opts ...LeaseOption) (*LeaseTimeToLiveResponse, error)

// List retrieves all leases.
List(ctx context.Context) (*LeaseListResponse, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

With this it's possible to do client.List(), which isn't clearly related to leases. Right now the pattern for RPCs is rpcname = LeaseX => client name = X. Maybe the RPC should be LeaseLeases so the client name can be Leases()?

kbs := make([][]byte, len(ks))
for i := range ks {
kbs[i] = []byte(ks[i])
switch {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this go through the http path so it can get the TTL? Can this RPC not do TTLs for the time being instead? Right now it won't work if the member is partitioned from the leader. Would prefer to keep it simple at first.

Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

does this need to go through raft at all?

@@ -56,6 +56,7 @@ This is a generated documentation. Please read the proto files for more.
| ------ | ------------ | ------------- | ----------- |
| LeaseGrant | LeaseGrantRequest | LeaseGrantResponse | LeaseGrant creates a lease which expires if the server does not receive a keepAlive within a given time to live period. All keys attached to the lease will be expired and deleted if the lease expires. Each expired key generates a delete event in the event history. |
| LeaseRevoke | LeaseRevokeRequest | LeaseRevokeResponse | LeaseRevoke revokes a lease. All keys attached to the lease will expire and be deleted. |
| LeaseList | LeaseListRequest | LeaseListResponse | LeaseList lists all existing leases. |
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be LeaseLeases so the rpc's still match the client interfaces?

@gyuho gyuho added the WIP label Aug 11, 2017
@gyuho gyuho force-pushed the lease-list branch 6 times, most recently from 85cba1c to 6ddea9a Compare August 12, 2017 13:44
@gyuho gyuho removed the WIP label Aug 12, 2017
@etcd-io etcd-io deleted a comment from codecov-io Aug 12, 2017
@gyuho
Copy link
Contributor Author

gyuho commented Aug 14, 2017

@heyitsanthony

does this need to go through raft at all?

I made the RPC only served by local node. PTAL. Thanks.

@gyuho
Copy link
Contributor Author

gyuho commented Aug 14, 2017

@heyitsanthony Changed LeaseItem to LeaseStatus, LeaseItems to Leases as discussed. PTAL. Thanks.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@8df2132). Click here to learn what that means.
The diff coverage is 76.31%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8358   +/-   ##
=========================================
  Coverage          ?   77.11%           
=========================================
  Files             ?      353           
  Lines             ?    28206           
  Branches          ?        0           
=========================================
  Hits              ?    21750           
  Misses            ?     4932           
  Partials          ?     1524
Impacted Files Coverage Δ
etcdctl/ctlv3/command/printer.go 43.26% <0%> (ø)
etcdctl/ctlv3/command/printer_fields.go 0% <0%> (ø)
etcdserver/v3_server.go 81.55% <100%> (ø)
proxy/grpcproxy/adapter/lease_client_adapter.go 100% <100%> (ø)
etcdctl/ctlv3/command/printer_simple.go 70% <100%> (ø)
etcdserver/api/v3rpc/lease.go 87.87% <40%> (ø)
clientv3/lease.go 91.88% <80%> (ø)
proxy/grpcproxy/lease.go 88.23% <81.81%> (ø)
etcdctl/ctlv3/command/lease_command.go 69.14% <83.33%> (ø)
lease/lessor.go 86.95% <93.75%> (ø)

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 8df2132...42dc170. Read the comment docs.

Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

lgtm after s/Leases() []leasepb.Lease/Leases() []*Lease/

lease/lessor.go Outdated
return leases
}

func (le *lessor) Leases() []leasepb.Lease {
Copy link
Contributor

Choose a reason for hiding this comment

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

just Leases() []*Lease? the call to this pulls out the fields into another structure anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants