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

[3.4] avoid closing a watch with ID 0 incorrectly #14562

Merged
merged 1 commit into from
Oct 10, 2022

Conversation

kafuu-chino
Copy link
Contributor

@kafuu-chino
Copy link
Contributor Author

@ahrtr @mitake I found unable to use Assert in 3.4 and use panic instead. Please help to confirm whether there will be any problem.

@ahrtr
Copy link
Member

ahrtr commented Oct 8, 2022

Thanks @kafuu-chino for backporting the PR.

Unfortunately, release-3.4 doesn't have integration test at all, so you have to remove the integration test case for now.

@ahrtr
Copy link
Member

ahrtr commented Oct 8, 2022

Please try to manually verify this PR indeed fixes the issue, thx

@kafuu-chino
Copy link
Contributor Author

Please try to manually verify this PR indeed fixes the issue, thx

In my local test, the token expires and watch can trigger normally.

Copy link
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

Thank you @kafuu-chino

@ahrtr
Copy link
Member

ahrtr commented Oct 8, 2022

cc @mitake @spzala to double confirm

@mitake
Copy link
Contributor

mitake commented Oct 9, 2022

@ahrtr @kafuu-chino I think just the path of integration test is different in release-3.4, and we can backport the test case like this? https://github.com/etcd-io/etcd/pull/14548/files#diff-93bc434c49bed544fa4df1bd0750fb22f98c12da81c75f8026520b4eed986108

@ahrtr
Copy link
Member

ahrtr commented Oct 9, 2022

@ahrtr @kafuu-chino I think just the path of integration test is different in release-3.4, and we can backport the test case like this? https://github.com/etcd-io/etcd/pull/14548/files#diff-93bc434c49bed544fa4df1bd0750fb22f98c12da81c75f8026520b4eed986108

Thanks @mitake for the reminder. release-3.4 indeed has integration test, and paths are as below,

etcd/test

Lines 185 to 188 in de11726

INTEGTESTPKG=("${REPO_PATH}/integration"
"${REPO_PATH}/client/integration"
"${REPO_PATH}/clientv3/integration"
"${REPO_PATH}/contrib/raftexample"

@kafuu-chino
Copy link
Contributor Author

@ahrtr @kafuu-chino I think just the path of integration test is different in release-3.4, and we can backport the test case like this? https://github.com/etcd-io/etcd/pull/14548/files#diff-93bc434c49bed544fa4df1bd0750fb22f98c12da81c75f8026520b4eed986108

Thanks @mitake for the reminder. release-3.4 indeed has integration test, and paths are as below,

etcd/test

Lines 185 to 188 in de11726

INTEGTESTPKG=("${REPO_PATH}/integration"
"${REPO_PATH}/client/integration"
"${REPO_PATH}/clientv3/integration"
"${REPO_PATH}/contrib/raftexample"

ok, I'll take a look today.

Signed-off-by: Kafuu Chino <KafuuChinoQ@gmail.com>

add test

1

1

1
@kafuu-chino
Copy link
Contributor Author

Integration test has been added. @ahrtr @mitake

@mitake
Copy link
Contributor

mitake commented Oct 10, 2022

LGTM, thanks @kafuu-chino @ahrtr !

@mitake mitake merged commit 57a27de into etcd-io:release-3.4 Oct 10, 2022
@ahrtr ahrtr mentioned this pull request Oct 29, 2022
4 tasks
@ahrtr ahrtr changed the title *: avoid closing a watch with ID 0 incorrectly [3.4] avoid closing a watch with ID 0 incorrectly Oct 29, 2022
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