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.

Sign up for GitHub

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

Jump to bottom

Revert "etcd server shouldn't wait for the ready notification infinitely on startup" #16677

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Revert "etcd server shouldn't wait for the ready notification infinitely on startup" #16677

merged 1 commit into from
Oct 4, 2023

Conversation

serathius
Copy link
Member

As proposed in #16666

This reverts commit 1713dc6.

cc @ahrtr @spzala @jmhbnz @wenjiaswe

@serathius serathius force-pushed the revert-13525 branch 2 times, most recently from f508bcc to d9940a5 Compare October 2, 2023 13:40
@serathius serathius marked this pull request as ready for review October 2, 2023 14:17
@serathius serathius marked this pull request as draft October 2, 2023 14:45
@serathius
Copy link
Member Author

Reverting to draft due to TestInPlaceRecovery failing. Potentially will need to redesign the test.

@serathius serathius force-pushed the revert-13525 branch 6 times, most recently from ee41c0f to 4fddf45 Compare October 3, 2023 19:18
@serathius
Revert "etcd server shouldn't wait for the ready notification infinit…
e31de5e 
…ely on startup"

This reverts commit 1713dc6.

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
@serathius serathius marked this pull request as ready for review October 3, 2023 20:21
@serathius
Copy link
Member Author

PTAL @ahrtr
I disabled the cluster_proxy testing for corrupt tests as I don't have energy to fight with the proxy.

jmhbnz
jmhbnz approved these changes Oct 3, 2023
View reviewed changes
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Based on discussion in #16666 I agree with decision to revert to prioritise consistency.

Defer decision on tests/e2e/corrupt_test.go //go:build !cluster_proxy to @ahrtr as I am not very familiar with this yet.

@ahrtr
Copy link
Member

ahrtr commented Oct 4, 2023

I disabled the cluster_proxy testing for corrupt tests as I don't have energy to fight with the proxy.

Part of the test cases in cluster_proxy.go were added before the 1713dc6, pls at least raise a followup.

@serathius serathius mentioned this pull request Oct 4, 2023
Open
@serathius
Copy link
Member Author

Done, #16690

ahrtr
ahrtr approved these changes Oct 4, 2023
View reviewed changes
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

thx

@serathius serathius merged commit 1c5289d into etcd-io:main Oct 4, 2023
27 checks passed
@serathius serathius mentioned this pull request Oct 4, 2023
Closed
@ahrtr ahrtr mentioned this pull request Oct 5, 2023
Merged
@serathius serathius mentioned this pull request Oct 17, 2023
Merged
@serathius serathius mentioned this pull request Nov 22, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrtr ahrtr ahrtr approved these changes

@jmhbnz jmhbnz jmhbnz approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@serathius @ahrtr @jmhbnz