Skip to content

Commit d390f9f

Browse files
JmPotatoJmPotato
committed
Address the comments
Signed-off-by: JmPotato <github@ipotato.me>
1 parent 0843c7f commit d390f9f

File tree

5 files changed

+52
-42
lines changed

5 files changed

+52
-42
lines changed

‎pkg/mcs/resourcemanager/server/apis/v1/api.go‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -267,10 +267,6 @@ func (s *Service) setKeyspaceServiceLimit(c *gin.Context) {
267267
c.String(http.StatusBadRequest, err.Error())
268268
return
269269
}
270-
if keyspaceIDValue == nil {
271-
c.String(http.StatusNotFound, fmt.Sprintf("keyspace not found with name: %s", keyspaceName))
272-
return
273-
}
274270
var req KeyspaceServiceLimitRequest
275271
if err := c.ShouldBindJSON(&req); err != nil {
276272
c.String(http.StatusBadRequest, err.Error())
@@ -291,6 +287,7 @@ func (s *Service) setKeyspaceServiceLimit(c *gin.Context) {
291287
// @Param keyspace_name path string true "Keyspace name"
292288
// @Success 200 {string} json format of rmserver.serviceLimiter
293289
// @Failure 400 {string} error
290+
// @Failure 404 {string} error
294291
// @Router /config/keyspace/service-limit/{keyspace_name} [get]
295292
func (s *Service) getKeyspaceServiceLimit(c *gin.Context) {
296293
keyspaceName := c.Param("keyspace_name")
@@ -299,10 +296,6 @@ func (s *Service) getKeyspaceServiceLimit(c *gin.Context) {
299296
c.String(http.StatusBadRequest, err.Error())
300297
return
301298
}
302-
if keyspaceIDValue == nil {
303-
c.String(http.StatusNotFound, fmt.Sprintf("keyspace not found with name: %s", keyspaceName))
304-
return
305-
}
306299
keyspaceID := keyspaceIDValue.GetValue()
307300
limiter := s.manager.GetKeyspaceServiceLimiter(keyspaceID)
308301
if limiter == nil {

‎pkg/mcs/resourcemanager/server/keyspace_manager.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ func newKeyspaceResourceGroupManager(keyspaceID uint32, storage endpoint.Resourc
6464
groups: make(map[string]*ResourceGroup),
6565
keyspaceID: keyspaceID,
6666
storage: storage,
67-
sl: newServiceLimiter(0),
67+
sl: newServiceLimiter(keyspaceID, 0),
6868
}
6969
}
7070

‎pkg/mcs/resourcemanager/server/manager.go‎

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -411,13 +411,17 @@ func (m *Manager) getKeyspaceNameByID(ctx context.Context, id uint32) (string, e
411411
return "", fmt.Errorf("got an empty keyspace name by id %d", id)
412412
}
413413
// Update the cache.
414-
m.Lock()
415-
m.keyspaceNameLookup[id] = loadedName
416-
m.keyspaceIDLookup[loadedName] = id
417-
m.Unlock()
414+
m.updateKeyspaceNameLookup(id, loadedName)
418415
return loadedName, nil
419416
}
420417

418+
func (m *Manager) updateKeyspaceNameLookup(id uint32, name string) {
419+
m.Lock()
420+
defer m.Unlock()
421+
m.keyspaceNameLookup[id] = name
422+
m.keyspaceIDLookup[name] = id
423+
}
424+
421425
// GetKeyspaceIDByName gets the keyspace ID by name.
422426
func (m *Manager) GetKeyspaceIDByName(ctx context.Context, name string) (*rmpb.KeyspaceIDValue, error) {
423427
if len(name) == 0 {
@@ -448,10 +452,7 @@ func (m *Manager) GetKeyspaceIDByName(ctx context.Context, name string) (*rmpb.K
448452
return nil, fmt.Errorf("keyspace not found with name: %s", name)
449453
}
450454
// Update the cache.
451-
m.Lock()
452-
m.keyspaceNameLookup[loadedID] = name
453-
m.keyspaceIDLookup[name] = loadedID
454-
m.Unlock()
455+
m.updateKeyspaceNameLookup(loadedID, name)
455456
return &rmpb.KeyspaceIDValue{Value: loadedID}, nil
456457
}
457458

‎pkg/mcs/resourcemanager/server/service_limit.go‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,10 @@ import (
1818
"math"
1919
"time"
2020

21+
"go.uber.org/zap"
22+
23+
"github.com/pingcap/log"
24+
2125
"github.com/tikv/pd/pkg/utils/syncutil"
2226
)
2327

@@ -36,14 +40,17 @@ type serviceLimiter struct {
3640
AvailableTokens float64 `json:"available_tokens"`
3741
// LastUpdate records the last time the limiter was updated.
3842
LastUpdate time.Time `json:"last_update"`
43+
// KeyspaceID is the keyspace ID of the keyspace that this limiter belongs to.
44+
keyspaceID uint32
3945
}
4046

41-
func newServiceLimiter(serviceLimit float64) *serviceLimiter {
47+
func newServiceLimiter(keyspaceID uint32, serviceLimit float64) *serviceLimiter {
4248
// The service limit should be non-negative.
4349
serviceLimit = math.Max(0, serviceLimit)
4450
return &serviceLimiter{
4551
ServiceLimit: serviceLimit,
4652
LastUpdate: time.Now(),
53+
keyspaceID: keyspaceID,
4754
}
4855
}
4956

@@ -79,6 +86,12 @@ func (krl *serviceLimiter) refillTokensLocked(now time.Time) {
7986
// Calculate the elapsed time since the last update.
8087
elapsed := now.Sub(krl.LastUpdate).Seconds()
8188
if elapsed < 0 {
89+
log.Warn("refill service limit tokens with negative elapsed time",
90+
zap.Uint32("keyspace_id", krl.keyspaceID),
91+
zap.Float64("service_limit", krl.ServiceLimit),
92+
zap.Float64("available_tokens", krl.AvailableTokens),
93+
zap.Float64("elapsed", elapsed),
94+
)
8295
return
8396
}
8497
// Add tokens based on the configured rate and the burst limit.
@@ -133,5 +146,6 @@ func (krl *serviceLimiter) Clone() *serviceLimiter {
133146
ServiceLimit: krl.ServiceLimit,
134147
AvailableTokens: krl.AvailableTokens,
135148
LastUpdate: krl.LastUpdate,
149+
keyspaceID: krl.keyspaceID,
136150
}
137151
}

‎pkg/mcs/resourcemanager/server/service_limit_test.go‎

Lines changed: 26 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -19,25 +19,27 @@ import (
1919
"time"
2020

2121
"github.com/stretchr/testify/require"
22+
23+
"github.com/tikv/pd/pkg/mcs/utils/constant"
2224
)
2325

2426
func TestNewServiceLimiter(t *testing.T) {
2527
re := require.New(t)
2628

2729
// Test creating a service limiter with positive limit
28-
limiter := newServiceLimiter(100.0)
30+
limiter := newServiceLimiter(constant.NullKeyspaceID, 100.0)
2931
re.NotNil(limiter)
3032
re.Equal(100.0, limiter.ServiceLimit)
3133
re.Equal(0.0, limiter.AvailableTokens)
3234

3335
// Test creating a service limiter with zero limit
34-
limiter = newServiceLimiter(0.0)
36+
limiter = newServiceLimiter(constant.NullKeyspaceID, 0.0)
3537
re.NotNil(limiter)
3638
re.Equal(0.0, limiter.ServiceLimit)
3739
re.Equal(0.0, limiter.AvailableTokens)
3840

3941
// Test creating a service limiter with negative limit
40-
limiter = newServiceLimiter(-10.0)
42+
limiter = newServiceLimiter(constant.NullKeyspaceID, -10.0)
4143
re.NotNil(limiter)
4244
re.Equal(0.0, limiter.ServiceLimit)
4345
re.Equal(0.0, limiter.AvailableTokens)
@@ -47,7 +49,7 @@ func TestRefillTokensLocked(t *testing.T) {
4749
re := require.New(t)
4850

4951
// Test refill with positive service limit
50-
limiter := newServiceLimiter(100.0)
52+
limiter := newServiceLimiter(constant.NullKeyspaceID, 100.0)
5153
baseTime := time.Now()
5254
limiter.LastUpdate = baseTime
5355
limiter.AvailableTokens = 50.0
@@ -99,29 +101,29 @@ func TestApplyServiceLimit(t *testing.T) {
99101
re.Equal(50.0, tokens)
100102

101103
// Test with zero service limit (no limit)
102-
limiter = newServiceLimiter(0.0)
104+
limiter = newServiceLimiter(constant.NullKeyspaceID, 0.0)
103105
now := time.Now()
104106
tokens = limiter.applyServiceLimit(now, 50.0)
105107
re.Equal(50.0, tokens)
106108

107109
// Test request within available tokens (need to set available tokens first)
108-
limiter = newServiceLimiter(100.0)
110+
limiter = newServiceLimiter(constant.NullKeyspaceID, 100.0)
109111
limiter.AvailableTokens = 100.0 // Manually set available tokens
110112
limiter.LastUpdate = now
111113
tokens = limiter.applyServiceLimit(now, 50.0)
112114
re.Equal(50.0, tokens)
113115
re.Equal(50.0, limiter.AvailableTokens) // 100 - 50 = 50
114116

115117
// Test request exactly equal to available tokens
116-
limiter = newServiceLimiter(100.0)
118+
limiter = newServiceLimiter(constant.NullKeyspaceID, 100.0)
117119
limiter.AvailableTokens = 100.0 // Manually set available tokens
118120
limiter.LastUpdate = now
119121
tokens = limiter.applyServiceLimit(now, 100.0)
120122
re.Equal(100.0, tokens)
121123
re.Equal(0.0, limiter.AvailableTokens)
122124

123125
// Test request exceeding available tokens
124-
limiter = newServiceLimiter(100.0)
126+
limiter = newServiceLimiter(constant.NullKeyspaceID, 100.0)
125127
limiter.LastUpdate = now
126128
limiter.AvailableTokens = 30.0
127129
tokens = limiter.applyServiceLimit(now, 80.0)
@@ -133,7 +135,7 @@ func TestApplyServiceLimitWithRefill(t *testing.T) {
133135
re := require.New(t)
134136

135137
// Test that refill happens before applying limit
136-
limiter := newServiceLimiter(100.0)
138+
limiter := newServiceLimiter(constant.NullKeyspaceID, 100.0)
137139
baseTime := time.Now()
138140
limiter.LastUpdate = baseTime
139141
limiter.AvailableTokens = 20.0
@@ -146,7 +148,7 @@ func TestApplyServiceLimitWithRefill(t *testing.T) {
146148
re.Equal(futureTime, limiter.LastUpdate)
147149

148150
// Test partial refill scenario
149-
limiter = newServiceLimiter(100.0)
151+
limiter = newServiceLimiter(constant.NullKeyspaceID, 100.0)
150152
limiter.LastUpdate = baseTime
151153
limiter.AvailableTokens = 10.0
152154

@@ -161,28 +163,28 @@ func TestServiceLimiterEdgeCases(t *testing.T) {
161163
re := require.New(t)
162164

163165
// Test with very small service limit
164-
limiter := newServiceLimiter(0.1)
166+
limiter := newServiceLimiter(constant.NullKeyspaceID, 0.1)
165167
limiter.AvailableTokens = 0.1 // Manually set available tokens
166168
limiter.LastUpdate = time.Now() // Set LastUpdate to current time to avoid refill
167169
now := time.Now()
168170
tokens := limiter.applyServiceLimit(now, 1.0)
169171
re.InDelta(0.1, tokens, 0.001) // Use InDelta to handle floating point precision
170172

171173
// Test with very large service limit
172-
limiter = newServiceLimiter(1000000.0)
174+
limiter = newServiceLimiter(constant.NullKeyspaceID, 1000000.0)
173175
limiter.AvailableTokens = 1000000.0 // Manually set available tokens
174176
tokens = limiter.applyServiceLimit(now, 500000.0)
175177
re.Equal(500000.0, tokens)
176178

177179
// Test with zero requested tokens
178-
limiter = newServiceLimiter(100.0)
180+
limiter = newServiceLimiter(constant.NullKeyspaceID, 100.0)
179181
limiter.AvailableTokens = 100.0 // Manually set available tokens
180182
tokens = limiter.applyServiceLimit(now, 0.0)
181183
re.Equal(0.0, tokens)
182184
re.Equal(100.0, limiter.AvailableTokens) // Should remain unchanged
183185

184186
// Test with fractional tokens
185-
limiter = newServiceLimiter(10.5)
187+
limiter = newServiceLimiter(constant.NullKeyspaceID, 10.5)
186188
limiter.LastUpdate = now
187189
limiter.AvailableTokens = 5.25
188190
tokens = limiter.applyServiceLimit(now, 7.75)
@@ -193,7 +195,7 @@ func TestSetServiceLimit(t *testing.T) {
193195
re := require.New(t)
194196

195197
// Test setting the same service limit (should be no-op)
196-
limiter := newServiceLimiter(100.0)
198+
limiter := newServiceLimiter(constant.NullKeyspaceID, 100.0)
197199
originalTokens := limiter.AvailableTokens
198200
originalUpdate := limiter.LastUpdate
199201

@@ -203,7 +205,7 @@ func TestSetServiceLimit(t *testing.T) {
203205
re.Equal(originalUpdate, limiter.LastUpdate) // Should remain unchanged
204206

205207
// Test increasing service limit
206-
limiter = newServiceLimiter(50.0)
208+
limiter = newServiceLimiter(constant.NullKeyspaceID, 50.0)
207209
baseTime := time.Now()
208210
limiter.LastUpdate = baseTime
209211
limiter.AvailableTokens = 30.0
@@ -214,7 +216,7 @@ func TestSetServiceLimit(t *testing.T) {
214216
re.True(limiter.LastUpdate.After(baseTime)) // Should update time
215217

216218
// Test decreasing service limit with available tokens exceeding new limit
217-
limiter = newServiceLimiter(100.0)
219+
limiter = newServiceLimiter(constant.NullKeyspaceID, 100.0)
218220
baseTime = time.Now()
219221
limiter.LastUpdate = baseTime
220222
limiter.AvailableTokens = 80.0
@@ -229,7 +231,7 @@ func TestSetServiceLimit(t *testing.T) {
229231
re.True(limiter.LastUpdate.After(baseTime)) // Should update time
230232

231233
// Test decreasing service limit with available tokens below new limit
232-
limiter = newServiceLimiter(100.0)
234+
limiter = newServiceLimiter(constant.NullKeyspaceID, 100.0)
233235
baseTime = time.Now()
234236
limiter.LastUpdate = baseTime
235237
limiter.AvailableTokens = 30.0
@@ -240,7 +242,7 @@ func TestSetServiceLimit(t *testing.T) {
240242
re.True(limiter.LastUpdate.After(baseTime)) // Should update time
241243

242244
// Test setting service limit to zero
243-
limiter = newServiceLimiter(100.0)
245+
limiter = newServiceLimiter(constant.NullKeyspaceID, 100.0)
244246
baseTime = time.Now()
245247
limiter.LastUpdate = baseTime
246248
limiter.AvailableTokens = 50.0
@@ -251,7 +253,7 @@ func TestSetServiceLimit(t *testing.T) {
251253
re.True(limiter.LastUpdate.After(baseTime)) // Should update time
252254

253255
// Test setting service limit from zero to positive (should NOT initialize available tokens)
254-
limiter = newServiceLimiter(0.0)
256+
limiter = newServiceLimiter(constant.NullKeyspaceID, 0.0)
255257
baseTime = time.Now()
256258
limiter.LastUpdate = baseTime
257259
limiter.AvailableTokens = 0.0
@@ -262,7 +264,7 @@ func TestSetServiceLimit(t *testing.T) {
262264
re.True(limiter.LastUpdate.After(baseTime)) // Should update time
263265

264266
// Test setting negative service limit (should be treated as zero)
265-
limiter = newServiceLimiter(100.0)
267+
limiter = newServiceLimiter(constant.NullKeyspaceID, 100.0)
266268
baseTime = time.Now()
267269
limiter.LastUpdate = baseTime
268270
limiter.AvailableTokens = 50.0
@@ -273,7 +275,7 @@ func TestSetServiceLimit(t *testing.T) {
273275
re.True(limiter.LastUpdate.After(baseTime)) // Should update time
274276

275277
// Test setting service limit with time elapsed (should trigger refill)
276-
limiter = newServiceLimiter(50.0)
278+
limiter = newServiceLimiter(constant.NullKeyspaceID, 50.0)
277279
baseTime = time.Now()
278280
limiter.LastUpdate = baseTime
279281
limiter.AvailableTokens = 20.0
@@ -288,7 +290,7 @@ func TestSetServiceLimit(t *testing.T) {
288290
re.True(limiter.LastUpdate.After(baseTime)) // Should update time
289291

290292
// Test setting a smaller service limit
291-
limiter = newServiceLimiter(1000.0)
293+
limiter = newServiceLimiter(constant.NullKeyspaceID, 1000.0)
292294
baseTime = time.Now()
293295
limiter.LastUpdate = baseTime
294296
limiter.AvailableTokens = 500.0
@@ -300,7 +302,7 @@ func TestSetServiceLimit(t *testing.T) {
300302
re.True(limiter.LastUpdate.After(baseTime)) // Should update time
301303

302304
// Test setting a larger service limit
303-
limiter = newServiceLimiter(10.0)
305+
limiter = newServiceLimiter(constant.NullKeyspaceID, 10.0)
304306
baseTime = time.Now()
305307
limiter.LastUpdate = baseTime
306308
limiter.AvailableTokens = 5.0

0 commit comments

Comments
 (0)