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

feature: implemented new lease-related api #66

Merged
merged 8 commits into from
Jul 13, 2020
Merged

Conversation

Yiyiyimu
Copy link
Contributor

@Yiyiyimu Yiyiyimu commented Jul 10, 2020

Fix #65

  1. implement lease-related api
  2. add test file
  3. add documentation

also fixed a naming typo

@CLAassistant
Copy link

CLAassistant commented Jul 10, 2020

CLA assistant check
All committers have signed the CLA.

@membphis membphis requested a review from nic-chen July 10, 2020 10:51
@membphis
Copy link
Contributor

@Yiyiyimu please take a look at the output of Travis-CI .

@Yiyiyimu
Copy link
Contributor Author

Hi @membphis I'm still working on the errors of other etcd versions. Is there anyway to set it to WIP? Or I just need to convert it to draft?

Also to deal with test in multi-version etcd locally, I'm trying to build dockerfile to automate the test process. Do you think that's necessary? Or that's not a big deal for others that building docker on my own is enough?

@membphis membphis changed the title feature: implemented new lease-related api [WIP] feature: implemented new lease-related api Jul 10, 2020
@membphis
Copy link
Contributor

@Yiyiyimu I have updated the title, you can continue your job now.

you can remove WIP after you complete this feature.

@Yiyiyimu
Copy link
Contributor Author

Thank you @membphis ! Do you have any suggestions on test of multi-version etcd locally?

@membphis
Copy link
Contributor

Thank you @membphis ! Do you have any suggestions on test of multi-version etcd locally?

we can run the specified etcd at our local machine, and try to find the bug. you can take a look at the travis script, it is useful.

@Yiyiyimu
Copy link
Contributor Author

Fix two problems:

  1. the different API of lease-related function in multi-version
  2. leases() is only supported after v3.4

Actually leases() is supported in v3.3.12, as etcd documentation shows. Since currently we deploy v3.3.0, I test it only in v3.4.0 and add another file lease3.4.t.

Considering this kind of problem could be common in the future, and this kind of naming is lack of readability, is there any need to build different branches in this repo according to different versions of etcd?

@Yiyiyimu Yiyiyimu changed the title [WIP] feature: implemented new lease-related api feature: implemented new lease-related api Jul 11, 2020
@Yiyiyimu
Copy link
Contributor Author

we can run the specified etcd at our local machine, and try to find the bug. you can take a look at the travis script, it is useful.

Thank you @membphis it works!

t/v3/lease.t Outdated
local data, err = etcd:get("/test")
check_res(data, err, "abc")

ngx.sleep(2.5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should get the val between seted and expired, such as 1 or 1.5 second after seated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

t/v3/lease.t Outdated
local res, err = etcd:grant(5)
check_res(res, err)

local data, err = etcd:set("/test", "abc", {prev_kv = true, lease = res.body.ID})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need more keys to test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Test on attaching multiple keys to one lease, and using revoke to delete them all.

t/v3/lease.t Outdated
check_res(data, err)

ngx.sleep(1)
if tonumber(data.body.TTL) < 5 then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why compare after slept? I’m confused about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's my fault. I add sleep after the comparison but forget to delete the comparison.

Also, I changed testing logic here from read TTL parameter to get key after default expired time.

t/v3/lease.t Outdated
local res, err = etcd:grant(5)
check_res(res, err)

local data, err = etcd:set("/test", "abc", {prev_kv = true, lease = res.body.ID})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need more keys too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Test on separately attaching two keys to two leases, keepalive one and to see if one alive and another one expired.

t/v3/lease3.4.t Show resolved Hide resolved
@Yiyiyimu
Copy link
Contributor Author

Yiyiyimu commented Jul 12, 2020

Improve test file in:

  1. multiple leases
  2. multiple keys attached to one lease
  3. the keys responded from timetolive are the same to granted (add decoding)
  4. manually assign ID when grant lease

It seems one v2 test file which I seems didn't change failed, I'll have a look on it.

@Yiyiyimu
Copy link
Contributor Author

It seems one v2 test file which I seems didn't change failed, I'll have a look on it.

Same with #12

@Yiyiyimu
Copy link
Contributor Author

Do we need to add the response structure of each API in doc, like only the body part, as mentioned in #37

For example timetolive

# +  body = {
# +    ID = "1",
# +    TTL = "2",
# +    grantedTTL = "3",
# +    header = {
# +      cluster_id = "14841639068965178418",
# +      member_id = "10276657743932975437",
# +      raft_term = "2",
# +      revision = "199"
# +    },
# +    keys = { "/test1" }
# +  },

and get

# +  body = {
# +    count = "1",
# +    header = {
# +      cluster_id = "14841639068965178418",
# +      member_id = "10276657743932975437",
# +      raft_term = "2",
# +      revision = "184"
# +    },
# +    kvs = { {
# +        create_revision = "184",
# +        key = "/test2",
# +        lease = "1",
# +        mod_revision = "184",
# +        value = "bcd",
# +        version = "1"
# +      } }
# +  },

@nic-chen
Copy link
Collaborator

it is better to add

@Yiyiyimu
Copy link
Contributor Author

it is better to add

Sure I'll create another PR for this change.

@nic-chen nic-chen merged commit 05eb12b into api7:master Jul 13, 2020
@membphis
Copy link
Contributor

@Yiyiyimu many thx

wfnuser pushed a commit to wfnuser/lua-resty-etcd that referenced this pull request Dec 15, 2020
* implement lease related func and test files

* add lease documentation

* fix naming typo of txn test file

* remove extra layer and fix formatting

* multiversion adaptation

* improved test file

* same with api7#12

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

Successfully merging this pull request may close these issues.

lease supported of etcd v3
4 participants