-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: use redis pubsub, transactions, locks #49
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@tituschewxj apart from the comments given I think the rest are good to go. Take a look at the comments, I just wanted to clarify some cases in the implementation:)
go processes.ReadMessages(ws, ctx, cancel) | ||
go processes.PerformMatching(matchRequest, context.Background(), matchFoundChannels) // Perform matching | ||
go processes.ReadMessages(ws, userCancel) | ||
go processes.PerformMatching(rdb, matchRequest, ctx, errorChan) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, each Websocket connection would trigger the PerformMatching goroutine, then within the PerformMatching goroutine if they fail to obtain lock would they retry? or just return and end ?
lock, err := databases.ObtainRedisLock(ctx)
if err != nil {
return
}
Possible case?:
user1 triggers perform matching
user2 and user3 tries to trigger perform matching but both fail to obtain lock so they return but user2 and user3 are a match
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they fail to retrieve the lock, they would retry every 100ms up to 100 times. If they still cannot obtain the lock after this than it just returns and it ends.
func ObtainRedisLock(ctx context.Context) (*redislock.Lock, error) {
// Retry every 100ms, for up-to 100x
backoff := redislock.LimitRetry(redislock.LinearBackoff(100*time.Millisecond), 100)
// Obtain lock with retry
lock, err := redisLock.Obtain(ctx, matchmakingRedisLock, time.Second, &redislock.Options{
RetryStrategy: backoff,
})
if err == redislock.ErrNotObtained {
fmt.Println("Could not obtain lock!")
return nil, err
} else if err != nil {
log.Fatalln(err)
return nil, err
}
return lock, err
}
So a possible improvement could be to add errorChan <- err
so that an error is returned.
lock, err := databases.ObtainRedisLock(ctx) | ||
if err != nil { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Links to the previous comment regarding the retry logic if it fails to obtain the lock
Summary
This PR implements:
Redis transactions. This allows either the matching to be successful or rolled back if something goes wrong.
Organize files and functions to be more structured
Refactors the matching algorithm
Uses Redis pub/sub to handle the sending of a match result instead of Go channels, to support inter-service communication
Uses Redis lock to ensure only one process modifies the database at any time to ensure correctness of matching algorithm
The server logs have also be slightly tweaked:
2024/10/23 16:25:59 Match found for user: Jennie
and2024/10/23 16:25:59 Match found for user: Timothy
have been removed as the information is already present elsewhere in the logs.Background
In Redis, multiple concurrent transactions can occur if they operate on different resources (i.e., different keys), but Redis still processes commands sequentially due to its single-threaded nature.
If multiple clients are interacting with different keys (i.e., different resources), there is no contention, and Redis processes the transactions or commands one after another without conflict.
Redis supports transactions through the use of the MULTI, EXEC, WATCH, and DISCARD commands. Transactions in Redis allow you to queue multiple commands to be executed atomically.
Current Issue
Currently, we are using a process level lock, which does not guarantee atomicity when multiple instance of the matching service is present. Hence the need for a database level atomic operations to be implemented.