cortexproject / cortex

A horizontally scalable, highly available, multi-tenant, long term Prometheus.
https://cortexmetrics.io/
Apache License 2.0
5.48k stars 801 forks source link

make memberlist kvstore to default #4971

Open songjiayang opened 2 years ago

songjiayang commented 2 years ago

Is your feature request related to a problem? Please describe.

currently the ring kvstore default value is consul, how about changing it to memberlist.

alvinlin123 commented 2 years ago

I think this idea is worth the discussing. What pros and cons do you think changing the default would have?

For me the cons is it would be a breaking change? Pros is better out-of-box experience ( no need to deploy extra service out-of-box).

songjiayang commented 2 years ago

Frankly speaking, this is indeed a break change. but when we want to use consul, we must configure consul_config, eg the address, so in fact, only a few people did not set store to consule.

if memberlist is default, we can use common memberlist_config, configuration will be more simpler.

memberlist isn't default:

memberlist:
  join_members: [cortex-1, cortex-2, cortex-3]

ingester:
  lifecycler:
    ring:
      kvstore:
        store: memberlist

store_gateway:
  sharding_enabled: true
  sharding_ring:
    kvstore:
      store: memberlist

ruler:
  enable_api: true
  enable_sharding: true
  poll_interval: 2s
  ring:
    kvstore:
      store: memberlist

memberlist is default:

memberlist:
  join_members: [cortex-1, cortex-2, cortex-3]

store_gateway:
  sharding_enabled: true

ruler:
  enable_api: true
  enable_sharding: true
  poll_interval: 2s
yeya24 commented 2 years ago

+1 to this change. I love the out of box experience and memberlist should be good enough for most of the users.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had any activity in the past 60 days. It will be closed in 15 days if no further activity occurs. Thank you for your contributions.

wgliang commented 3 months ago

Will this continue to move forward?

alanprot commented 3 months ago

I think make sense to change it to default!

Im afraid about the breaking change indeed.. we may break customer on next release if they did not explicitly configure this? Even though i think its unlikely that we configure the other consul config but not the type.

wgliang commented 3 months ago

I think we can start by supporting optional use of memberlist, and as for changing the default, we can transition to a later version.

wgliang commented 3 months ago

To some extent, customers do not want to have external dependencies (Etcd...risk) in production environments. We are way behind the other one project in some aspects, you know what I mean.

yeya24 commented 3 months ago

If we want to change it anyway then there is breaking change for sure so I am not concerned about breaking customers. I think I am ok to change memberlist as the default.

That being said, there is nothing blocking this change. It is always welcome to create a PR to Cortex if this is the feature you are looking for.

alanprot commented 3 months ago

I think we can start by supporting optional use of memberlist, and as for changing the default, we can transition to a later version.

Cortex already support memberlist.

wgliang commented 3 months ago

I think we can start by supporting optional use of memberlist, and as for changing the default, we can transition to a later version.

Cortex already support memberlist.

Yeah, I missed it. But tracker kv store only supports consul and etcd.