Scale down feature #1
Conversation
controller/controller.go
Outdated
|
|
||
| l.Debug("accessing redis") | ||
| pipe := redisClient.TxPipeline() | ||
| sReady := pipe.SPopN(models.GetRoomStatusSetRedisKey(scheduler.Name, models.StatusReady), int64(amount)) |
There was a problem hiding this comment.
I never knew redis had a SPopN method. Cool 😎
There was a problem hiding this comment.
If your are calling just one method in redis there's no need for a txpipeline.
There was a problem hiding this comment.
Yeah, I need access to github.com/topfreegames/extensions to add this new method to RedisClient interface
There was a problem hiding this comment.
Done! You should be able to commit and push to github.com/topfreegames/extensions
controller/controller.go
Outdated
| sReady := pipe.SPopN(models.GetRoomStatusSetRedisKey(scheduler.Name, models.StatusReady), int64(amount)) | ||
| _, err := pipe.Exec() | ||
| if err != nil { | ||
| l.WithError(err).Error("scale down error") |
There was a problem hiding this comment.
use a better message here so we can distinguish this error from the one below: "failed to retrieve ready rooms from redis" maybe?
controller/controller.go
Outdated
| @@ -272,6 +349,14 @@ func deleteSchedulerHelper(logger logrus.FieldLogger, mr *models.MixedMetricsRep | |||
| } | |||
| } | |||
|
|
|||
| err = mr.WithSegment(models.SegmentDelete, func() error { | |||
There was a problem hiding this comment.
Why did you move the scheduler removal from db to the first operation? It was the last beacause if something failed, the watcher would recreate the other parts (namespace, redis info, etc) and if the user still wanted to delete the scheduler he could call the endpoint again.
If you first delete it from the db and then attempt to do the remaining ops when there's a failure we'll have junk info (maybe a namespace, junk info in redis, etc) that we won't be able to detect easily.
Please move it back to the last op and add a comment just above it explaining why this is needed.
There was a problem hiding this comment.
That makes sense.
I changed it because when I deleted a scheduler, the namespace was recreated without services and pods. I thought it was because the line on DB was not deleted, but actually just moving CreateNamespaceIfNecessary solved the problem.
I will move it back.
There was a problem hiding this comment.
Well, the watcher should recreate it with services and pods. This might be a bug so please take some time to make sure it works as expected.
There was a problem hiding this comment.
It does, the problem was calling CreateNamespaceIfNecessary before GetSchedulerScalingInfo
| }) | ||
|
|
||
| err = controller.CreateNamespaceIfNecessary( |
There was a problem hiding this comment.
Ok this makes sense. We'll just create the namespace if the scheduler still exists in the db.
henrod
force-pushed
the
feat/scale-down
branch
3 times, most recently
from
23b8a40
to
6575424
Compare
| @@ -332,13 +403,17 @@ func deleteSchedulerHelper(logger logrus.FieldLogger, mr *models.MixedMetricsRep | |||
| } | |||
| } | |||
|
|
|||
| // Delete from DB must be the last operation because | |||
Scale Down
How it's done