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

Added: '-force-prune' to remove workers not on the heartbeat list #8

Merged
merged 1 commit into from
Jul 30, 2021

Conversation

xescugc
Copy link
Member

@xescugc xescugc commented Jun 30, 2021

This will remove workers thare could be stuck but it's not backwards compatible if enabled

Copy link

@xlr-8 xlr-8 left a comment

Choose a reason for hiding this comment

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

For which previous version would it fail?
Couldn't we check that the "prune workers" don't represent 100% of the fleet?
And if so, not do it - to avoid breaking the ones using the old lib?

CHANGELOG.md Outdated
### Added

- New flag `-force-prune` to remove workers not on the heartbeat list
([PR #8](https://github.com/cycloidio/goworker/issues/8))
Copy link

Choose a reason for hiding this comment

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

Right link?

// This option is not compatible with older
// versions of Resque (any port) as older versions
// may not have heartbeat so this would delete
// real working workers.
Copy link

Choose a reason for hiding this comment

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

How would that work for onpremise setup? Anything that should be of concern?

Copy link
Member Author

Choose a reason for hiding this comment

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

The main issue would be to use it in conjuntion with running workers, the idea would be to restart all workers with the new version if this flag want's to be used. Old ones are ones that do not have the heartbeat logic which since 0.15 is enabled

Copy link

Choose a reason for hiding this comment

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

So as long as the flag is added to updated workers we're good?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes if the workers are using this lib with v0.15=> it'll work fine, if not this will "kill" any old worker.

@xescugc
Copy link
Member Author

xescugc commented Jul 1, 2021

For which previous version would it fail?
Couldn't we check that the "prune workers" don't represent 100% of the fleet?
And if so, not do it - to avoid breaking the ones using the old lib?

The option is disabled by default for this reason. It'll fail if the version used of resque does not support heartbeat as all workers then would be considered as dead if they are not registered on the heartbeat list.

This flag is also just for cleanup, it does not need to be enabled all time normally.

Fixed the comment.

Copy link

@xlr-8 xlr-8 left a comment

Choose a reason for hiding this comment

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

LGTM if my understanding is correct

// This option is not compatible with older
// versions of Resque (any port) as older versions
// may not have heartbeat so this would delete
// real working workers.
Copy link

Choose a reason for hiding this comment

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

So as long as the flag is added to updated workers we're good?

This will remove workers thare could be stuck but it's not backwards compatible if enabled
@xlr-8
Copy link

xlr-8 commented Jul 28, 2021

What are we waiting for?

@xescugc xescugc merged commit df57256 into master Jul 30, 2021
@xescugc xescugc deleted the fg-new-flags branch July 30, 2021 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants