Address PR comments

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{})
 		}
 

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(),

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)

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 {

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 {

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.

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:"-"`