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

nsqd: return error if pause state is not changed #1051

Closed
wants to merge 3 commits into from

Conversation

ploxiln
Copy link
Member

@ploxiln ploxiln commented Jul 7, 2018

so the caller of pause or unpause can know if the topic or channel
was already paused or unpaused

also avoids a little bit of work if no change

a humble idea, just to solicit opinions, could probably use better error strings

@mreiferson
Copy link
Member

I guess it's useful to propagate this information?

@ploxiln
Copy link
Member Author

ploxiln commented Jul 7, 2018

Yeah I though it could be useful info to get in the response - sort of inspired by the corner-case of maybe having a script retry pause a few times until it takes effect. Maybe not worth it.

There's also the interesting bit where Pause() / UnPause() return type error, but (until this change) it is always nil. They could return the previous state as a bool instead. Or nothing. Dunno if any software out there really uses nsqd guts as a library, probably not, and we're breaking "api behavior" with the topic pause-or-start changes anyway ...

@mreiferson
Copy link
Member

Ready to land this?

@ploxiln
Copy link
Member Author

ploxiln commented Jul 8, 2018

hang on, I can add a proper test pretty easily

@ploxiln
Copy link
Member Author

ploxiln commented Jul 9, 2018

I'm thinking about maybe returning 200 but with a body that says e.g. "already paused" somehow. May be less disruptive to existing scripts. Which may or may not even exist. I dunno.

Anyway I'd like to defer this PR until after #1053 is resolved.

so the caller of pause or unpause can know if the topic or channel
was already paused or unpaused

also avoids a little bit of work if no change
@ploxiln ploxiln force-pushed the pause_check_change branch from da93b51 to 31e0aab Compare July 14, 2018 18:40
@ploxiln
Copy link
Member Author

ploxiln commented Jul 14, 2018

I pushed up a "non-error" variant. But I'm thinking it's best to just close this until someone asks for this feature. (I don't really need it myself.)

@ploxiln ploxiln closed this Jul 14, 2018
@ploxiln ploxiln deleted the pause_check_change branch August 27, 2019 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants