Skip to content

Conversation

@kwonoj
Copy link
Member

@kwonoj kwonoj commented Mar 14, 2016

Description:

This PR amends flush behavior in QueueScheduler, reset active status even if queued action throws, so further queued action can be executed.

Related issue (if exists):

closes #1464

  • Little bit unsure if this change's legit, or might be better recommended way. Will wait for comment.

@trxcllnt
Copy link
Member

LGTM

@ccrowhurstram
Copy link

It's seems this is relatively core / basic functionality. Therefore, surprised that the existing tests didn't pick this up.

Shouldn't there be a test to add? That would aid as a reminder that this kind of stuff needs testing?

@kwonoj
Copy link
Member Author

kwonoj commented Mar 14, 2016

@ccrowhurstram I also noticed there isn't test coverage for QueueScheduler, started to working on those. I definitely agree test coverage should cover most of core features.

@ccrowhurstram
Copy link

Excellent

@kwonoj
Copy link
Member Author

kwonoj commented Mar 14, 2016

Closing this by #1469 .

@kwonoj kwonoj closed this Mar 14, 2016
@kwonoj kwonoj deleted the fix-queuescheduler branch March 14, 2016 17:29
@benlesh
Copy link
Member

benlesh commented Mar 14, 2016

Whoa, whoa, hold on... did we perf test this? This is going to deoptimize the flush function. I have two other PRs proposing other changes around this.

@kwonoj
Copy link
Member Author

kwonoj commented Mar 14, 2016

@Blesh , this PR isn't merged, after your PR propsed. it's simply closed.

@benlesh
Copy link
Member

benlesh commented Mar 14, 2016

I see.

@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

interval seems unusable after stream fails

4 participants