From cac0c5f446132acbd488ef15f53dca8e2d8371c3 Mon Sep 17 00:00:00 2001
From: Max Holland <max@livepeer.org>
Date: Tue, 16 Jul 2024 17:31:23 +0100
Subject: [PATCH] review comments

---
 clients/manifest.go               |  3 +++
 config/storage_backup_url_test.go | 30 ++++++++++++++++++++++++++++++
 main.go                           |  2 +-
 test/steps/ffmpeg.go              |  7 ++++---
 4 files changed, 38 insertions(+), 4 deletions(-)
 create mode 100644 config/storage_backup_url_test.go

diff --git a/clients/manifest.go b/clients/manifest.go
index 7532ab190..54a1f9b57 100644
--- a/clients/manifest.go
+++ b/clients/manifest.go
@@ -169,6 +169,9 @@ func downloadManifest(requestID, sourceManifestOSURL string) (playlist m3u8.Play
 		rc, err := GetFile(context.Background(), requestID, sourceManifestOSURL, dStorage)
 		if err != nil {
 			if time.Since(start) > 10*time.Second && errors.IsObjectNotFound(err) {
+				// bail out of the retries earlier for not found errors because it will be quite a common scenario
+				// where the backup manifest does not exist and we don't want to wait the whole 50s of retries for
+				// every recording job
 				return backoff.Permanent(err)
 			}
 			return err
diff --git a/config/storage_backup_url_test.go b/config/storage_backup_url_test.go
new file mode 100644
index 000000000..cd4c9432c
--- /dev/null
+++ b/config/storage_backup_url_test.go
@@ -0,0 +1,30 @@
+package config
+
+import "testing"
+
+func TestGetStorageBackupURL(t *testing.T) {
+	StorageFallbackURLs = map[string]string{"https://storj.livepeer.com/catalyst-recordings-com/hls": "https://google.livepeer.com/catalyst-recordings-com/hls"}
+	tests := []struct {
+		name   string
+		urlStr string
+		want   string
+	}{
+		{
+			name:   "should replace",
+			urlStr: "https://storj.livepeer.com/catalyst-recordings-com/hls/foo",
+			want:   "https://google.livepeer.com/catalyst-recordings-com/hls/foo",
+		},
+		{
+			name:   "should not replace",
+			urlStr: "https://blah.livepeer.com/catalyst-recordings-com/hls/foo",
+			want:   "",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := GetStorageBackupURL(tt.urlStr); got != tt.want {
+				t.Errorf("GetStorageBackupURL() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
diff --git a/main.go b/main.go
index 4b5f193bf..b2164760e 100644
--- a/main.go
+++ b/main.go
@@ -117,7 +117,7 @@ func main() {
 	fs.StringVar(&cli.EncryptKey, "encrypt", "", "Key for encrypting network traffic within Serf. Must be a base64-encoded 32-byte key.")
 	fs.StringVar(&cli.VodDecryptPublicKey, "catalyst-public-key", "", "Public key of the catalyst node for encryption")
 	fs.StringVar(&cli.VodDecryptPrivateKey, "catalyst-private-key", "", "Private key of the catalyst node for encryption")
-	config.CommaMapFlag(fs, &cli.StorageFallbackURLs, "storage-fallback-urls", map[string]string{}, `Comma-separated map of primary to backup storage URLs. If a file fails downloading from one of the primary storages (detected by prefix), it will fallback to the corresponding backup URL after having the prefix replaced`)
+	config.CommaMapFlag(fs, &cli.StorageFallbackURLs, "storage-fallback-urls", map[string]string{}, `Comma-separated map of primary to backup storage URLs. If a file fails downloading from one of the primary storages (detected by prefix), it will fallback to the corresponding backup URL after having the prefix replaced. E.g. https://storj.livepeer.com/catalyst-recordings-com/hls=https://google.livepeer.com/catalyst-recordings-com/hls`)
 	fs.StringVar(&cli.GateURL, "gate-url", "http://localhost:3004/api/access-control/gate", "Address to contact playback gating API for access control verification")
 	fs.StringVar(&cli.DataURL, "data-url", "http://localhost:3004/api/data", "Address of the Livepeer Data Endpoint")
 	config.InvertedBoolFlag(fs, &cli.MistTriggerSetup, "mist-trigger-setup", true, "Overwrite Mist triggers with the ones built into catalyst-api")
diff --git a/test/steps/ffmpeg.go b/test/steps/ffmpeg.go
index dde1a2cbe..49ef43963 100644
--- a/test/steps/ffmpeg.go
+++ b/test/steps/ffmpeg.go
@@ -31,11 +31,13 @@ func (s *StepContext) AllOfTheSourceSegmentsAreWrittenToStorageWithinSeconds(num
 	session := osDriver.NewSession(filepath.Join(s.latestRequestID, "source"))
 
 	var latestNumSegments int
-	for x := 0; x < secs; x++ {
+	for x := 0; x < secs; x++ { // retry loop
+		if x > 0 {
+			time.Sleep(time.Second)
+		}
 		page, err := session.ListFiles(context.Background(), "", "")
 		if err != nil {
 			log.Println("failed to list files: ", err)
-			time.Sleep(time.Second)
 			continue
 		}
 
@@ -43,7 +45,6 @@ func (s *StepContext) AllOfTheSourceSegmentsAreWrittenToStorageWithinSeconds(num
 		if latestNumSegments == numSegments+1 {
 			return nil
 		}
-		time.Sleep(time.Second)
 	}
 	return fmt.Errorf("did not find the expected number of source segments in %s (wanted %d, got %d)", s.SourceOutputDir, numSegments, latestNumSegments)
 }