diff --git a/ring/basic_lifecycler.go b/ring/basic_lifecycler.go index ff5726c11..b9a95af87 100644 --- a/ring/basic_lifecycler.go +++ b/ring/basic_lifecycler.go @@ -316,7 +316,7 @@ func (l *BasicLifecycler) registerInstance(ctx context.Context) error { // Always overwrite the instance in the ring (even if already exists) because some properties // may have changed (stated, tokens, zone, address) and even if they didn't the heartbeat at // least did. - // TODO: add support for read-only state and timestamp. + // Note: this lifecycler does not support read only instances for now. instanceDesc = ringDesc.AddIngester(l.cfg.ID, l.cfg.Addr, l.cfg.Zone, tokens, state, registeredAt, false, time.Time{}) return ringDesc, true, nil }) @@ -443,8 +443,8 @@ func (l *BasicLifecycler) updateInstance(ctx context.Context, update func(*Desc, // Due to how shuffle sharding work, the missing instance for some period of time could have cause // a resharding of tenants among instances: to guarantee query correctness we need to update the // registration timestamp to current time. + // Note: this lifecycler does not support read only instances for now. registeredAt := time.Now() - // TODO: add support for read-only state and timestamp. instanceDesc = ringDesc.AddIngester(l.cfg.ID, l.cfg.Addr, l.cfg.Zone, l.GetTokens(), l.GetState(), registeredAt, false, time.Time{}) } diff --git a/ring/basic_lifecycler_test.go b/ring/basic_lifecycler_test.go index 3ba67999d..914221a6c 100644 --- a/ring/basic_lifecycler_test.go +++ b/ring/basic_lifecycler_test.go @@ -107,7 +107,7 @@ func TestBasicLifecycler_RegisterOnStart(t *testing.T) { initialInstanceDesc: &InstanceDesc{ Addr: "1.1.1.1", State: ACTIVE, - Tokens: Tokens{6, 7, 8, 9, 10}, + Tokens: Tokens{1, 2, 3, 4, 5}, RegisteredTimestamp: time.Now().Add(-time.Hour).Unix(), ReadOnly: true, ReadOnlyUpdatedTimestamp: time.Now().Unix(), diff --git a/ring/lifecycler.go b/ring/lifecycler.go index 7bc9c35fc..1ff80d99a 100644 --- a/ring/lifecycler.go +++ b/ring/lifecycler.go @@ -675,9 +675,8 @@ func (i *Lifecycler) initRing(ctx context.Context) error { instanceDesc, ok := ringDesc.Ingesters[i.ID] if !ok { - // The instance doesn't exist in the ring, so it's safe to set the registered timestamp - // as of now. now := time.Now() + // The instance doesn't exist in the ring, so it's safe to set the registered timestamp as of now. i.setRegisteredAt(now) // Clear read-only state, and set last update time to "now". i.setReadOnlyState(false, now) diff --git a/ring/model.go b/ring/model.go index bb988e0ee..08f1d7690 100644 --- a/ring/model.go +++ b/ring/model.go @@ -55,7 +55,11 @@ func timeToUnixSecons(t time.Time) int64 { // AddIngester adds the given ingester to the ring. Ingester will only use supplied tokens, // any other tokens are removed. func (d *Desc) AddIngester(id, addr, zone string, tokens []uint32, state InstanceState, registeredAt time.Time, readOnly bool, readOnlyUpdated time.Time) InstanceDesc { - inst := InstanceDesc{ + if d.Ingesters == nil { + d.Ingesters = map[string]InstanceDesc{} + } + + ingester := InstanceDesc{ Id: id, Addr: addr, Timestamp: time.Now().Unix(), @@ -67,15 +71,6 @@ func (d *Desc) AddIngester(id, addr, zone string, tokens []uint32, state Instanc ReadOnlyUpdatedTimestamp: timeToUnixSecons(readOnlyUpdated), } - d.AddInstance(id, inst) - return inst -} - -func (d *Desc) AddInstance(id string, ingester InstanceDesc) InstanceDesc { - if d.Ingesters == nil { - d.Ingesters = map[string]InstanceDesc{} - } - d.Ingesters[id] = ingester return ingester } @@ -151,14 +146,6 @@ func (i *InstanceDesc) GetRegisteredAt() time.Time { return time.Unix(i.RegisteredTimestamp, 0) } -func (i *InstanceDesc) SetRegisteredAt(t time.Time) { - if t.IsZero() { - i.RegisteredTimestamp = 0 - } else { - i.RegisteredTimestamp = t.Unix() - } -} - // GetReadOnlyState returns the read-only state and timestamp of last read-only state update. func (i *InstanceDesc) GetReadOnlyState() (bool, time.Time) { if i == nil { diff --git a/ring/model_test.go b/ring/model_test.go index ae39b1ba0..2f6c00043 100644 --- a/ring/model_test.go +++ b/ring/model_test.go @@ -290,6 +290,16 @@ func TestDesc_RingsCompare(t *testing.T) { r2: &Desc{Ingesters: map[string]InstanceDesc{"ing1": {Addr: "addr1", State: ACTIVE, RegisteredTimestamp: 2}}}, expected: Different, }, + "same single instance, different read only flag": { + r1: &Desc{Ingesters: map[string]InstanceDesc{"ing1": {Addr: "addr1"}}}, + r2: &Desc{Ingesters: map[string]InstanceDesc{"ing1": {Addr: "addr1", ReadOnly: true}}}, + expected: Different, + }, + "same single instance, different read only timestamp": { + r1: &Desc{Ingesters: map[string]InstanceDesc{"ing1": {Addr: "addr1", ReadOnlyUpdatedTimestamp: time.Time{}.Unix()}}}, + r2: &Desc{Ingesters: map[string]InstanceDesc{"ing1": {Addr: "addr1", ReadOnlyUpdatedTimestamp: time.Now().Unix()}}}, + expected: Different, + }, "instance in different zone": { r1: &Desc{Ingesters: map[string]InstanceDesc{"ing1": {Addr: "addr1", Zone: "one"}}}, r2: &Desc{Ingesters: map[string]InstanceDesc{"ing1": {Addr: "addr1", Zone: "two"}}}, @@ -315,11 +325,6 @@ func TestDesc_RingsCompare(t *testing.T) { r2: &Desc{Ingesters: map[string]InstanceDesc{"ing1": {Addr: "addr1", Tokens: []uint32{1, 2, 4}}}}, expected: Different, }, - "same number of instances, using different IDs": { - r1: &Desc{Ingesters: map[string]InstanceDesc{"ing1": {Addr: "addr1", Tokens: []uint32{1, 2, 3}}}}, - r2: &Desc{Ingesters: map[string]InstanceDesc{"ing2": {Addr: "addr1", Tokens: []uint32{1, 2, 3}}}}, - expected: Different, - }, } for testName, testData := range tests { diff --git a/ring/ring.pb.go b/ring/ring.pb.go index f0c4af024..f976b7e99 100644 --- a/ring/ring.pb.go +++ b/ring/ring.pb.go @@ -128,9 +128,10 @@ type InstanceDesc struct { RegisteredTimestamp int64 `protobuf:"varint,8,opt,name=registered_timestamp,json=registeredTimestamp,proto3" json:"registered_timestamp,omitempty"` // ID of the instance. This value is the same as the key in the ingesters map in Desc. Id string `protobuf:"bytes,9,opt,name=id,proto3" json:"id,omitempty"` - // Timestamp when the read_only flag was updated. This is used to find other instances that could have possibly - // owned a specific token in the past on the write path, due to *this* instance being read-only. - // This value should only increase. + // Unix timestamp (with seconds precision) of when the read_only flag was updated. This + // is used to find other instances that could have possibly owned a specific token in + // the past on the write path, due to *this* instance being read-only. This value should + // only increase. ReadOnlyUpdatedTimestamp int64 `protobuf:"varint,10,opt,name=read_only_updated_timestamp,json=readOnlyUpdatedTimestamp,proto3" json:"read_only_updated_timestamp,omitempty"` // Indicates whether this instance is read only. // Read-only instances go through standard state changes, and special handling is applied to them diff --git a/ring/ring.proto b/ring/ring.proto index b93df6f81..7795e8493 100644 --- a/ring/ring.proto +++ b/ring/ring.proto @@ -45,9 +45,10 @@ message InstanceDesc { // ID of the instance. This value is the same as the key in the ingesters map in Desc. string id = 9; - // Timestamp when the read_only flag was updated. This is used to find other instances that could have possibly - // owned a specific token in the past on the write path, due to *this* instance being read-only. - // This value should only increase. + // Unix timestamp (with seconds precision) of when the read_only flag was updated. This + // is used to find other instances that could have possibly owned a specific token in + // the past on the write path, due to *this* instance being read-only. This value should + // only increase. int64 read_only_updated_timestamp = 10; // Indicates whether this instance is read only. diff --git a/ring/ring_http.go b/ring/ring_http.go index 9d62a121f..67249e2b4 100644 --- a/ring/ring_http.go +++ b/ring/ring_http.go @@ -41,8 +41,8 @@ type ingesterDesc struct { Address string `json:"address"` HeartbeatTimestamp time.Time `json:"timestamp"` RegisteredTimestamp time.Time `json:"registered_timestamp"` - ReadOnly bool `json:"readonly"` - ReadOnlyUpdatedTimestamp time.Time `json:"readonly_updated_timestamp"` + ReadOnly bool `json:"read_only"` + ReadOnlyUpdatedTimestamp time.Time `json:"read_only_updated_timestamp"` Zone string `json:"zone"` Tokens []uint32 `json:"tokens"` NumTokens int `json:"-"`