diff --git a/pkg/statestore/redis.go b/pkg/statestore/redis.go index 1188de4..04901bf 100644 --- a/pkg/statestore/redis.go +++ b/pkg/statestore/redis.go @@ -2,7 +2,6 @@ package statestore import ( "context" - "errors" "fmt" "strconv" "strings" @@ -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 @@ -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. diff --git a/pkg/statestore/redis_test.go b/pkg/statestore/redis_test.go index 7d8c1c8..331ee61 100644 --- a/pkg/statestore/redis_test.go +++ b/pkg/statestore/redis_test.go @@ -148,10 +148,10 @@ 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) @@ -159,43 +159,27 @@ func TestTicketTTL(t *testing.T) { _, 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) { diff --git a/pkg/statestore/statestore.go b/pkg/statestore/statestore.go index 4cac305..ee8ebb9 100644 --- a/pkg/statestore/statestore.go +++ b/pkg/statestore/statestore.go @@ -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)