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

Rollback the pull/14080: which fixed the potential deadlock #17058

Merged
merged 1 commit into from
Dec 5, 2023

Conversation

ahrtr
Copy link
Member

@ahrtr ahrtr commented Dec 4, 2023

Rollback #14080

Please read #16035 (comment)

@ahrtr
Copy link
Member Author

ahrtr commented Dec 4, 2023

cc @chaochn47 @serathius @qsyqian

@serathius
Copy link
Member

serathius commented Dec 4, 2023

Could you add a test to confirm that there is no longer a leak and prevent future regressions?

@ahrtr
Copy link
Member Author

ahrtr commented Dec 5, 2023

Could you add a test to confirm that there is no longer a leak and prevent future regressions?

This PR just rollback, can we do whatever other thing separately?

@serathius
Copy link
Member

It's a partial rollback, which means it should be treated as normal change.

@ahrtr
Copy link
Member Author

ahrtr commented Dec 5, 2023

I don't understand why we are wasting time on such small thing.

The part I did not rollback is a trivial change. If you are insist on, I can totally rollback the PR.

@ahrtr ahrtr force-pushed the rollback_lease_lock_20231204 branch from 1db3fa0 to 9a6eeb5 Compare December 5, 2023 10:51
@ahrtr ahrtr changed the title Partially rollback the pull/14080: which fixed the potential deadlock Rollback the pull/14080: which fixed the potential deadlock Dec 5, 2023
@ahrtr
Copy link
Member Author

ahrtr commented Dec 5, 2023

Completely rollbacked, pls let me know if you still have any concern. @serathius

@serathius
Copy link
Member

I don't understand why we are wasting time on such small thing.

Reverts are only trivial if they revert us to exactly the same stable point as before. Partial revert moves us to new unknown place. Possibly it is an improvement, but to verify we need to either review it thoroughly or have test shows the improvement.

The revert was motivated by issue that issue slipped our review, meaning it's as trivial as originally assumed. We didn't have any tests before, we should invest in them to reduce the dependency on review.

@ahrtr ahrtr merged commit 4ece556 into etcd-io:main Dec 5, 2023
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.

2 participants