Skip to content

Commit 9d4fadb

Browse files
krajoramakrajorama
committed
review comment fixes
1 parent 713b77e commit 9d4fadb

File tree

4 files changed

+42
-28
lines changed

4 files changed

+42
-28
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,6 @@
2323
* [ENHANCEMENT] Memberlist: prepare the data to send on the write before starting counting the elapsed time for `-memberlist.packet-write-timeout`, in order to reduce chances we hit the timeout when sending a packet to other node. #89
2424
* [ENHANCEMENT] flagext: for cases such as `DeprecatedFlag()` that need a logger, add RegisterFlagsWithLogger. #80
2525
* [ENHANCEMENT] Added option to BasicLifecycler to keep instance in the ring when stopping. #97
26+
* [ENHANCEMENT] Add WaitRingTokensStability function to ring, to be able to wait on ring stability excluding allowed state transitions. #95
2627
* [BUGFIX] spanlogger: Support multiple tenant IDs. #59
2728
* [BUGFIX] Memberlist: fixed corrupted packets when sending compound messages with more than 255 messages or messages bigger than 64KB. #85
28-
* [ENHANCEMENT] Add WaitRingTokensStability function to ring, to be able to wait on ring stability excluding allowed state transitions. #95

‎ring/replication_set_test.go‎

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -242,17 +242,16 @@ func TestReplicationSet_Do(t *testing.T) {
242242
}
243243
}
244244

245-
func TestHasReplicationSetChangedWithoutStateIgnoresTimeStampAndState(t *testing.T) {
246-
// Only testing difference to underlying Equal function
247-
rs1 := ReplicationSet{
245+
var (
246+
replicationSetChangesInitialState = ReplicationSet{
248247
Instances: []InstanceDesc{
249248
{Addr: "127.0.0.1"},
250249
{Addr: "127.0.0.2"},
251250
{Addr: "127.0.0.3"},
252251
},
253252
}
254-
tests := map[string]struct {
255-
rs ReplicationSet
253+
replicationSetChangesTestCases = map[string]struct {
254+
nextState ReplicationSet
256255
expectHasReplicationSetChanged bool
257256
expectHasReplicationSetChangedWithoutState bool
258257
}{
@@ -291,10 +290,22 @@ func TestHasReplicationSetChangedWithoutStateIgnoresTimeStampAndState(t *testing
291290
true,
292291
},
293292
}
294-
for testName, testData := range tests {
293+
)
294+
295+
func TestHasReplicationSetChanged_IgnoresTimeStamp(t *testing.T) {
296+
// Only testing difference to underlying Equal function
297+
for testName, testData := range replicationSetChangesTestCases {
298+
t.Run(testName, func(t *testing.T) {
299+
assert.Equal(t, testData.expectHasReplicationSetChanged, HasReplicationSetChanged(replicationSetChangesInitialState, testData.nextState), "HasReplicationSetChanged wrong result")
300+
})
301+
}
302+
}
303+
304+
func TestHasReplicationSetChangedWithoutState_IgnoresTimeStampAndState(t *testing.T) {
305+
// Only testing difference to underlying Equal function
306+
for testName, testData := range replicationSetChangesTestCases {
295307
t.Run(testName, func(t *testing.T) {
296-
assert.Equal(t, testData.expectHasReplicationSetChanged, HasReplicationSetChanged(rs1, testData.rs), "HasReplicationSetChanged wrong result")
297-
assert.Equal(t, testData.expectHasReplicationSetChangedWithoutState, HasReplicationSetChangedWithoutState(rs1, testData.rs), "HasReplicationSetChangedWithoutState wrong result")
308+
assert.Equal(t, testData.expectHasReplicationSetChangedWithoutState, HasReplicationSetChangedWithoutState(replicationSetChangesInitialState, testData.nextState), "HasReplicationSetChangedWithoutState wrong result")
298309
})
299310
}
300311
}

‎ring/util.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,7 @@ func WaitRingStability(ctx context.Context, r *Ring, op Operation, minStability,
100100
}
101101

102102
// WaitRingTokensStability waits for the Ring to be unchanged at
103-
// least for minStability time period, excluding transitioninig between
103+
// least for minStability time period, excluding transitioning between
104104
// allowed states (e.g. JOINING->ACTIVE if allowed by op).
105105
// This can be used to avoid wasting resources on moving data around
106106
// due to multiple changes in the Ring.

‎ring/util_test.go‎

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ func TestGenerateTokens(t *testing.T) {
7979
}
8080
}
8181

82-
func TestGenerateTokensIgnoresOldTokens(t *testing.T) {
82+
func TestGenerateTokens_IgnoresOldTokens(t *testing.T) {
8383
first := GenerateTokens(1000000, nil)
8484
second := GenerateTokens(1000000, first)
8585

@@ -119,7 +119,7 @@ func createStartingRing() *Ring {
119119
return ring
120120
}
121121

122-
func TestWaitRingStabilityShouldReturnAsSoonAsMinStabilityIsReachedOnNoChanges(t *testing.T) {
122+
func TestWaitRingStability_ShouldReturnAsSoonAsMinStabilityIsReachedOnNoChanges(t *testing.T) {
123123
t.Parallel()
124124

125125
const (
@@ -133,10 +133,11 @@ func TestWaitRingStabilityShouldReturnAsSoonAsMinStabilityIsReachedOnNoChanges(t
133133
require.NoError(t, WaitRingStability(context.Background(), ring, Reporting, minStability, maxWaiting))
134134
elapsedTime := time.Since(startTime)
135135

136-
assert.InDelta(t, minStability, elapsedTime, float64(2*time.Second))
136+
assert.GreaterOrEqual(t, elapsedTime.Milliseconds(), minStability.Milliseconds())
137+
assert.Less(t, elapsedTime.Milliseconds(), 2*minStability.Milliseconds())
137138
}
138139

139-
func TestWaitRingTokensStabilityShouldReturnAsSoonAsMinStabilityIsReachedOnNoChanges(t *testing.T) {
140+
func TestWaitRingTokensStability_ShouldReturnAsSoonAsMinStabilityIsReachedOnNoChanges(t *testing.T) {
140141
t.Parallel()
141142

142143
const (
@@ -150,7 +151,8 @@ func TestWaitRingTokensStabilityShouldReturnAsSoonAsMinStabilityIsReachedOnNoCha
150151
require.NoError(t, WaitRingTokensStability(context.Background(), ring, Reporting, minStability, maxWaiting))
151152
elapsedTime := time.Since(startTime)
152153

153-
assert.InDelta(t, minStability, elapsedTime, float64(2*time.Second))
154+
assert.GreaterOrEqual(t, elapsedTime.Milliseconds(), minStability.Milliseconds())
155+
assert.Less(t, elapsedTime.Milliseconds(), 2*minStability.Milliseconds())
154156
}
155157

156158
func addInstanceAfterSomeTime(ring *Ring, addInstanceAfter time.Duration) {
@@ -170,7 +172,7 @@ func addInstanceAfterSomeTime(ring *Ring, addInstanceAfter time.Duration) {
170172
}()
171173
}
172174

173-
func TestWaitRingStabilityShouldReturnOnceMinStabilityOfInstancesHasBeenReached(t *testing.T) {
175+
func TestWaitRingStability_ShouldReturnOnceMinStabilityOfInstancesHasBeenReached(t *testing.T) {
174176
t.Parallel()
175177

176178
const (
@@ -192,7 +194,7 @@ func TestWaitRingStabilityShouldReturnOnceMinStabilityOfInstancesHasBeenReached(
192194
assert.LessOrEqual(t, elapsedTime.Milliseconds(), (minStability + addInstanceAfter + 3*time.Second).Milliseconds())
193195
}
194196

195-
func TestWaitRingTokensStabilityShouldReturnOnceMinStabilityOfInstancesHasBeenReached(t *testing.T) {
197+
func TestWaitRingTokensStability_ShouldReturnOnceMinStabilityOfInstancesHasBeenReached(t *testing.T) {
196198
t.Parallel()
197199

198200
const (
@@ -241,7 +243,7 @@ func addInstancesPeriodically(ring *Ring) chan struct{} {
241243
return done
242244
}
243245

244-
func TestWaitRingStabilityShouldReturnErrorIfInstancesAddedAndMaxWaitingIsReached(t *testing.T) {
246+
func TestWaitRingStability_ShouldReturnErrorIfInstancesAddedAndMaxWaitingIsReached(t *testing.T) {
245247
t.Parallel()
246248

247249
const (
@@ -258,10 +260,10 @@ func TestWaitRingStabilityShouldReturnErrorIfInstancesAddedAndMaxWaitingIsReache
258260
require.Equal(t, context.DeadlineExceeded, WaitRingStability(context.Background(), ring, Reporting, minStability, maxWaiting))
259261
elapsedTime := time.Since(startTime)
260262

261-
assert.InDelta(t, maxWaiting, elapsedTime, float64(2*time.Second))
263+
assert.GreaterOrEqual(t, elapsedTime.Milliseconds(), maxWaiting.Milliseconds())
262264
}
263265

264-
func TestWaitRingTokensStabilityShouldReturnErrorIfInstancesAddedAndMaxWaitingIsReached(t *testing.T) {
266+
func TestWaitRingTokensStability_ShouldReturnErrorIfInstancesAddedAndMaxWaitingIsReached(t *testing.T) {
265267
t.Parallel()
266268

267269
const (
@@ -278,7 +280,7 @@ func TestWaitRingTokensStabilityShouldReturnErrorIfInstancesAddedAndMaxWaitingIs
278280
require.Equal(t, context.DeadlineExceeded, WaitRingTokensStability(context.Background(), ring, Reporting, minStability, maxWaiting))
279281
elapsedTime := time.Since(startTime)
280282

281-
assert.InDelta(t, maxWaiting, elapsedTime, float64(2*time.Second))
283+
assert.GreaterOrEqual(t, elapsedTime.Milliseconds(), maxWaiting.Milliseconds())
282284
}
283285

284286
// Keep changing the ring in a way to avoid repeating the same set of states for at least 2 sec
@@ -311,7 +313,7 @@ func changeStatePeriodically(ring *Ring) chan struct{} {
311313
return done
312314
}
313315

314-
func TestWaitRingStabilityShouldReturnErrorIfInstanceStateIsChangingAndMaxWaitingIsReached(t *testing.T) {
316+
func TestWaitRingStability_ShouldReturnErrorIfInstanceStateIsChangingAndMaxWaitingIsReached(t *testing.T) {
315317
t.Parallel()
316318

317319
const (
@@ -329,10 +331,10 @@ func TestWaitRingStabilityShouldReturnErrorIfInstanceStateIsChangingAndMaxWaitin
329331
require.Equal(t, context.DeadlineExceeded, WaitRingStability(context.Background(), ring, Reporting, minStability, maxWaiting))
330332
elapsedTime := time.Since(startTime)
331333

332-
assert.InDelta(t, maxWaiting, elapsedTime, float64(2*time.Second))
334+
assert.GreaterOrEqual(t, elapsedTime.Milliseconds(), maxWaiting.Milliseconds())
333335
}
334336

335-
func TestWaitRingTokensStabilityShouldReturnOnceMinStabilityOfInstancesHasBeenReachedWhileStateCanChange(t *testing.T) {
337+
func TestWaitRingTokensStability_ShouldReturnOnceMinStabilityOfInstancesHasBeenReachedWhileStateCanChange(t *testing.T) {
336338
t.Parallel()
337339

338340
const (
@@ -350,10 +352,11 @@ func TestWaitRingTokensStabilityShouldReturnOnceMinStabilityOfInstancesHasBeenRe
350352
require.Equal(t, nil, WaitRingTokensStability(context.Background(), ring, Reporting, minStability, maxWaiting))
351353
elapsedTime := time.Since(startTime)
352354

353-
assert.InDelta(t, minStability, elapsedTime, float64(2*time.Second))
355+
assert.GreaterOrEqual(t, elapsedTime.Milliseconds(), minStability.Milliseconds())
356+
assert.Less(t, elapsedTime.Milliseconds(), 2*minStability.Milliseconds())
354357
}
355358

356-
func TestWaitInstanceStateTimeout(t *testing.T) {
359+
func TestWaitInstanceState_Timeout(t *testing.T) {
357360
t.Parallel()
358361

359362
const (
@@ -373,7 +376,7 @@ func TestWaitInstanceStateTimeout(t *testing.T) {
373376
ring.AssertCalled(t, "GetInstanceState", instanceID)
374377
}
375378

376-
func TestWaitInstanceStateTimeoutOnError(t *testing.T) {
379+
func TestWaitInstanceState_TimeoutOnError(t *testing.T) {
377380
t.Parallel()
378381

379382
const (
@@ -393,7 +396,7 @@ func TestWaitInstanceStateTimeoutOnError(t *testing.T) {
393396
ring.AssertCalled(t, "GetInstanceState", instanceID)
394397
}
395398

396-
func TestWaitInstanceStateExitsAfterActualStateEqualsState(t *testing.T) {
399+
func TestWaitInstanceState_ExitsAfterActualStateEqualsState(t *testing.T) {
397400
t.Parallel()
398401

399402
const (

0 commit comments

Comments
 (0)