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

clientv3: Disconnected Linearized Reads #8341

Merged
merged 6 commits into from
Aug 6, 2017
Merged

Conversation

visheshnp
Copy link
Contributor

No description provided.

"github.com/coreos/etcd/pkg/testutil"
)

func TestLeasingPutGet1(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/TestLeasingPutGet1/TestLeasingPutGet/? Or does 1 mean something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh no, it doesn't. Sorry, I forgot to edit the test names.

@gyuho
Copy link
Contributor

gyuho commented Aug 3, 2017

@visheshnp can you fix the commit titles as well? s/clientv3 :/clientv3: /

@visheshnp visheshnp force-pushed the leasing-pr branch 2 times, most recently from e30025f to 2ccbf65 Compare August 3, 2017 23:07
@codecov-io
Copy link

codecov-io commented Aug 3, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #8341   +/-   ##
=========================================
  Coverage          ?   77.16%           
=========================================
  Files             ?      353           
  Lines             ?    28019           
  Branches          ?        0           
=========================================
  Hits              ?    21621           
  Misses            ?     4882           
  Partials          ?     1516
Impacted Files Coverage Δ
clientv3/kv.go 97.18% <100%> (ø)
etcdmain/grpc_proxy.go 62.43% <33.33%> (ø)
clientv3/op.go 72.8% <84.61%> (ø)
clientv3/leasing/cache.go 87.22% <87.22%> (ø)
clientv3/leasing/txn.go 89.68% <89.68%> (ø)
clientv3/leasing/kv.go 90.7% <90.7%> (ø)
clientv3/leasing/util.go 98.33% <98.33%> (ø)

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 366f538...d3716b8. Read the comment docs.

@heyitsanthony
Copy link
Contributor

@visheshnp lgtm once tests pass, but the commits that touch the leasing package should be prefixed with leasing: not clientv3:. Also the leasing commits should be squashed into a single leasing: commit (same for integration, one integration: commit`) before this can be merged. Thanks

// limitations under the License.

// Package leasing is a clientv3 wrapper that provides the client exclusive write access to a key by acquiring a lease and be lineraizably
// served locally. This leasing layer can either directly wrap the etcd client
Copy link
Member

Choose a reason for hiding this comment

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

single space between either and directly?

// //Output: 123 (served locally)
//
// Lease Revocation:
// If a client writes to the key owned by the leasing client, then the leasing client gives up its lease allowing the client to modify the key.
Copy link
Member

Choose a reason for hiding this comment

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

single space owned between by?

@visheshnp visheshnp force-pushed the leasing-pr branch 2 times, most recently from bae0d12 to 6391b8d Compare August 4, 2017 03:44
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

@xiang90 xiang90 merged commit b39891e into etcd-io:master Aug 6, 2017
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.

6 participants