Skip to content

clientv3: remove v3.WithFirstKey() in Barrier.Wait()#16156

Merged
ahrtr merged 1 commit intoetcd-io:mainfrom
kensou97:fix-barrier
Jul 6, 2023
Merged

clientv3: remove v3.WithFirstKey() in Barrier.Wait()#16156
ahrtr merged 1 commit intoetcd-io:mainfrom
kensou97:fix-barrier

Conversation

@kensou97
Copy link
Copy Markdown
Contributor

fix the unexpected blocking when using Barrier.Wait(), e.g. NewBarrier(client, "a").Wait() will block if key "a" is not existed but "a0" is existed, but it should return immediately.

Copy link
Copy Markdown
Member

@chaochn47 chaochn47 left a comment

Choose a reason for hiding this comment

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

The barrier only expects one key is locked. So the change makes sense to me. 

// WithFirstKey gets the lexically first key in the request range.
func WithFirstKey() []OpOption { return withTop(SortByKey, SortAscend) }
// withTop gets the first key over the get's prefix given a sort order
func withTop(target SortTarget, order SortOrder) []OpOption {
	return []OpOption{WithPrefix(), WithSort(target, order), WithLimit(1)}
}

Could you add a test in https://github.com/etcd-io/etcd/blob/main/tests/integration/clientv3/experimental/recipes/v3_barrier_test.go?

@kensou97
Copy link
Copy Markdown
Contributor Author

kensou97 commented Jul 1, 2023

The barrier only expects one key is locked. So the change makes sense to me. 

// WithFirstKey gets the lexically first key in the request range.
func WithFirstKey() []OpOption { return withTop(SortByKey, SortAscend) }
// withTop gets the first key over the get's prefix given a sort order
func withTop(target SortTarget, order SortOrder) []OpOption {
	return []OpOption{WithPrefix(), WithSort(target, order), WithLimit(1)}
}

Could you add a test in https://github.com/etcd-io/etcd/blob/main/tests/integration/clientv3/experimental/recipes/v3_barrier_test.go?

added, please help to review.

Copy link
Copy Markdown
Member

@chaochn47 chaochn47 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @kensou97!

@chaochn47
Copy link
Copy Markdown
Member

/cc @ahrtr @serathius to start the CI test

Comment thread tests/integration/clientv3/experimental/recipes/v3_barrier_test.go Outdated
@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Jul 4, 2023

Overall looks good to me. Please resolve the minor comment above, and also squash the commits.

fix the unexpected blocking when using Barrier.Wait(), e.g.
NewBarrier(client, "a").Wait() will block if key "a" is not existed but "a0" is existed, but it should return immediately.

Signed-off-by: zhangwenkang <zwenkang@vmware.com>
@kensou97
Copy link
Copy Markdown
Contributor Author

kensou97 commented Jul 4, 2023

Overall looks good to me. Please resolve the minor comment above, and also squash the commits.

done

Copy link
Copy Markdown
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @kensou97

@ahrtr ahrtr merged commit 93bfdba into etcd-io:main Jul 6, 2023
@ahrtr
Copy link
Copy Markdown
Member

ahrtr commented Jul 6, 2023

@kensou97 can you please backport this PR to 3.5?

@kensou97
Copy link
Copy Markdown
Contributor Author

kensou97 commented Jul 6, 2023

@kensou97 can you please backport this PR to 3.5?

ok, please help review #16188

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

Development

Successfully merging this pull request may close these issues.

3 participants