Skip to content

Commit

Permalink
Frontend does not acquire locks.
Browse files Browse the repository at this point in the history
Frontend no longer calls deIndexTickets because Backend's GetTickets does it asynchronously.
Therefore, Frontend no longer needs to acquire the ticket index lock, which improves Redis performance.
  • Loading branch information
castaneai committed May 30, 2024
1 parent bf02f6b commit 33974f3
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 34 deletions.
10 changes: 4 additions & 6 deletions pkg/statestore/redis.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package statestore

import (
"context"
"errors"
"fmt"
"strconv"
"strings"
Expand Down Expand Up @@ -161,10 +160,9 @@ func (s *RedisStore) GetTicket(ctx context.Context, ticketID string) (*pb.Ticket
}
ticket, err := s.getTicket(ctx, s.client, ticketID)
if err != nil {
if errors.Is(err, ErrTicketNotFound) {
// If the ticket has been deleted by TTL, it is deleted from the ticket index as well.
_ = s.deIndexTickets(ctx, []string{ticketID})
}
// ErrTicketNotFound is here in the deletion by TTL,
// and it looks like deIndexTickets is necessary, but it is not
// because deIndex is done behind the scenes by Backend's GetTickets call.
return nil, err
}
return ticket, nil
Expand Down Expand Up @@ -204,7 +202,7 @@ func (s *RedisStore) GetAssignment(ctx context.Context, ticketID string) (*pb.As

// GetActiveTicketIDs may also retrieve tickets deleted by TTL.
// This is because the ticket index and Ticket data are stored in separate keys.
// The next `GetTicket` or `GetTickets` call will resolve this inconsistency.
// The next `GetTickets` call will resolve this inconsistency.
func (s *RedisStore) GetActiveTicketIDs(ctx context.Context, limit int64) ([]string, error) {
// Acquire a lock to prevent multiple backends from fetching the same Ticket.
// In order to avoid race conditions with other Ticket Index changes, get tickets and set them to pending state should be done atomically.
Expand Down
40 changes: 12 additions & 28 deletions pkg/statestore/redis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,54 +148,38 @@ func TestTicketTTL(t *testing.T) {
require.Equal(t, id, ticket.Id)
}

_, err := store.GetTicket(ctx, "test1")
require.Error(t, err, ErrTicketNotFound)

mustCreateTicket("test1")
test1, err := store.GetTicket(ctx, "test1")
require.NoError(t, err)
require.Equal(t, "test1", test1.Id)

// "test1" has been deleted by TTL
mr.FastForward(ticketTTL + 1*time.Second)

_, err = store.GetTicket(ctx, "test1")
require.Error(t, err, ErrTicketNotFound)

activeTicketIDs, err := store.GetActiveTicketIDs(ctx, defaultGetTicketLimit)
require.NoError(t, err)
require.NotContains(t, activeTicketIDs, "test1")

mustCreateTicket("test2")
mustCreateTicket("test3")

ts, err := store.GetTickets(ctx, []string{"test2", "test3"})
require.NoError(t, err)
require.ElementsMatch(t, ticketIDs(ts), []string{"test2", "test3"})

// "test2" and "test3" have been deleted by TTL
mr.FastForward(ticketTTL + 1*time.Second)

// "test4" remains as it has not passed TTL
mustCreateTicket("test4")

// The ActiveTicketIDs may still contain the ID of a ticket that was deleted by TTL.
// This is because the ticket index and Ticket data are stored in separate keys.

// In this example, "test2" and "test3" were deleted by TTL, but remain in the ticket index.
activeTicketIDs, err = store.GetActiveTicketIDs(ctx, defaultGetTicketLimit)
// In this example, "test1" was deleted by TTL, but remain in the ticket index.
activeTicketIDs, err := store.GetActiveTicketIDs(ctx, defaultGetTicketLimit)
require.NoError(t, err)
require.ElementsMatch(t, activeTicketIDs, []string{"test2", "test3", "test4"})
err = store.ReleaseTickets(ctx, []string{"test2", "test3", "test4"})
require.ElementsMatch(t, activeTicketIDs, []string{"test1", "test2"})

err = store.ReleaseTickets(ctx, []string{"test1", "test2"})
require.NoError(t, err)

// `GetTickets` call will resolve inconsistency.
ts, err = store.GetTickets(ctx, []string{"test2", "test3", "test4"})
ts, err := store.GetTickets(ctx, []string{"test1"})
require.NoError(t, err)
require.ElementsMatch(t, ticketIDs(ts), []string{"test4"})
require.Empty(t, ts)

// Because we called GetTickets, "test2" and "test3" which were deleted by TTL,
// were deleted from the ticket index as well.
// Because we called GetTickets, "test1" was deleted from the ticket index.
activeTicketIDs, err = store.GetActiveTicketIDs(ctx, defaultGetTicketLimit)
require.NoError(t, err)
require.ElementsMatch(t, activeTicketIDs, []string{"test4"})
require.ElementsMatch(t, activeTicketIDs, []string{"test2"})
}

func TestConcurrentFetchActiveTickets(t *testing.T) {
Expand Down
2 changes: 2 additions & 0 deletions pkg/statestore/statestore.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ var (
type StateStore interface {
CreateTicket(ctx context.Context, ticket *pb.Ticket) error
DeleteTicket(ctx context.Context, ticketID string) error
// GetTicket is an API to retrieve the status of a single ticket and is called from Frontend.
GetTicket(ctx context.Context, ticketID string) (*pb.Ticket, error)
// GetTickets on the other hand, retrieves multiple tickets at once and is intended to be called from Backend.
GetTickets(ctx context.Context, ticketIDs []string) ([]*pb.Ticket, error)
GetAssignment(ctx context.Context, ticketID string) (*pb.Assignment, error)
GetActiveTicketIDs(ctx context.Context, limit int64) ([]string, error)
Expand Down

0 comments on commit 33974f3

Please sign in to comment.