Merge pull request #27 from castaneai/frontend-no-nolonger-calls-deIn…

…dexTickets

Frontend does not acquire locks on GetTicket.

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.

pkg/statestore/redis_test.go

@@ -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) {

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)