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

proposal: Deprecate/Remove gossip clustering for Thanos. #493

Merged
merged 2 commits into from
Sep 12, 2018

Conversation

bwplotka
Copy link
Member

This is quite a major change, so please give us feedback!

Rendered view: https://github.com/improbable-eng/thanos/blob/gossip-remove-proposal/docs/proposals/gossip-removal.md

PTAL: @brancz @povilasv @fabxc @domgreen and others.

@mxinden @stuartnelson3 this may interest you.


## Goals

* Deprecate gossip protocol or remove it immediately (TODO: Gather input here!)
Copy link
Member Author

Choose a reason for hiding this comment

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

I need some feedback on how to handle that. I feel like just removal would the best option in terms of maintainance and simplicity. However if someone already is running Thanos in production this might distrupt a lot. So maybe do slow deprecate until some "notice time"?

Copy link
Member

Choose a reason for hiding this comment

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

My personal take is that Thanos is a very young project and this change is overall for the better so I think a hard break here is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend to agree, publicly declare deprication and the plan as we work on file-sd and then remove once file-sd has been proven out fully.
Users can always take old images from docker hub if needed whilst we move forward.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated doc accordingly

* Alternatively just remove it? (we are still in RC)

### Backwards compatibility

Copy link
Contributor

Choose a reason for hiding this comment

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

Could take the gossip code and move to a new project that writes to file-sd if people are really keen on keeping it going. However, I would leave this to the community if they feel it is needed as supporting this would be as much work as supporting gossip within thanos

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting idea!

Copy link
Member Author

Choose a reason for hiding this comment

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

will add

@povilasv
Copy link
Member

👍 Great proposal. I think if we can avoid gossip, we should definitely do that.

@xjewer
Copy link
Contributor

xjewer commented Aug 29, 2018

Do you have any particular plans/dates for implementing this? Do you need any help?

@xjewer
Copy link
Contributor

xjewer commented Aug 29, 2018

Worth mentioning in the proposal adding DNS SD as well #492 (comment)

@bwplotka bwplotka force-pushed the gossip-remove-proposal branch 2 times, most recently from 165b032 to 1f2d913 Compare August 31, 2018 12:00
Copy link
Member

@brancz brancz left a comment

Choose a reason for hiding this comment

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

lgtm

* Add File Service Discovery (SD): https://github.com/improbable-eng/thanos/issues/492
* Remove gossip from the documentation, be clear what talks with what (!)
* Deprecate gossip in code.
* Remove gossip code and flags AFTER some time (month?)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think there is any time to wait. Once file discovery is implemented and documented it should just be removed directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool.

@ivan-valkov you were asking how blocking File SD issue is. (:

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are all in agreement that we should move forward with this approach 👍

Copy link
Contributor

@domgreen domgreen left a comment

Choose a reason for hiding this comment

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

It seems from discussions and proposed direction that moving away from gossip in favour of using FileSD is favoured by all.

Lets update to accepted and merge 👍

@brancz
Copy link
Member

brancz commented Sep 3, 2018

👍

This is quite a major change, so please give us feedback!

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
…sip implementation removal.

Signed-off-by: Bartek Plotka <bwplotka@gmail.com>
@bwplotka
Copy link
Member Author

bwplotka commented Sep 3, 2018

Moved status to approved and added requirement on FileSD before gossip implementation removal.

@bwplotka bwplotka merged commit ad04aed into master Sep 12, 2018
@bwplotka bwplotka deleted the gossip-remove-proposal branch September 12, 2018 18:55
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.

5 participants