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

Revert "cluster: wait on servers closing before disconnect" #1945

Closed
wants to merge 1 commit into from

Conversation

sam-github
Copy link
Contributor

This reverts commit 9c0a1b8.

CI is timing out, work is continuing in #1934

I merged #1400 without running CI on it, and after removing what seemed an irrelevant setTimeout(). Sorry, I won't do that again.

This reverts commit 9c0a1b8.

CI is timing out, work is continuing in
nodejs#1934
@trevnorris
Copy link
Contributor

Eh, we've all been there. My record is patching my own patch two hours after commit for switching a && with ||.

On the side, have one final test running on CI that should tell us whether it was the test that's faulty or the patch.

@brendanashworth brendanashworth added the cluster Issues and PRs related to the cluster subsystem. label Jun 11, 2015
@trevnorris
Copy link
Contributor

FYI, @jbergstroem has helped me confirm that the test is what's wrong. Specifically when run with test.py. When run on its own seems to complete fine. Here's the output: https://gist.github.com/jbergstroem/9b9f11f0e43e21cf5cb7

IOW let's give this at least another day of troubleshooting before reverting the change.

@cjihrig
Copy link
Contributor

cjihrig commented Jun 11, 2015

LGTM. If nothing is found in the next day, feel free to land.

@Olegas
Copy link
Contributor

Olegas commented Jun 11, 2015

Why not just to fix the test?

@cjihrig
Copy link
Contributor

cjihrig commented Jun 11, 2015

@Olegas that is the plan. The revert is a fallback.

@Olegas
Copy link
Contributor

Olegas commented Jun 11, 2015

@cjihrig They are already fixed here in #1934. This is how they are originally implemented. There are some weird timing issues I think we can address in another issue later.

@trevnorris
Copy link
Contributor

I'm still working on troubleshooting the test so don't land this yet. I'll report back a little later today.

@Olegas Unfortunately if a test fails then it's considered faulty. If it can't be figured out then the entire patch will be reverted. This is just standard protocol to allow the patch to be hammered out without causing any failures on CI. Re-landing it again once it's more solid won't be an issue at all.

@sam-github
Copy link
Contributor Author

Unnecessary, test fixed in #1953

@sam-github sam-github closed this Jun 13, 2015
@sam-github sam-github deleted the revert-1400 branch April 17, 2017 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants