From e1b269e956e955dd1dfb012f40270d73f8329092 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 14 Nov 2024 20:04:20 -0800 Subject: [PATCH] Remove transaction for archive download (#32186) Since there is a status column in the database, the transaction is unnecessary when downloading an archive. The transaction is blocking database operations, especially with SQLite. Replace #27563 --- services/repository/archiver/archiver.go | 33 ++++++++----------- services/repository/archiver/archiver_test.go | 12 +++---- 2 files changed, 19 insertions(+), 26 deletions(-) diff --git a/services/repository/archiver/archiver.go b/services/repository/archiver/archiver.go index 01c58f0ce4..c33369d047 100644 --- a/services/repository/archiver/archiver.go +++ b/services/repository/archiver/archiver.go @@ -68,7 +68,7 @@ func (e RepoRefNotFoundError) Is(err error) bool { } // NewRequest creates an archival request, based on the URI. The -// resulting ArchiveRequest is suitable for being passed to ArchiveRepository() +// resulting ArchiveRequest is suitable for being passed to Await() // if it's determined that the request still needs to be satisfied. func NewRequest(repoID int64, repo *git.Repository, uri string) (*ArchiveRequest, error) { r := &ArchiveRequest{ @@ -151,13 +151,14 @@ func (aReq *ArchiveRequest) Await(ctx context.Context) (*repo_model.RepoArchiver } } +// doArchive satisfies the ArchiveRequest being passed in. Processing +// will occur in a separate goroutine, as this phase may take a while to +// complete. If the archive already exists, doArchive will not do +// anything. In all cases, the caller should be examining the *ArchiveRequest +// being returned for completion, as it may be different than the one they passed +// in. func doArchive(ctx context.Context, r *ArchiveRequest) (*repo_model.RepoArchiver, error) { - txCtx, committer, err := db.TxContext(ctx) - if err != nil { - return nil, err - } - defer committer.Close() - ctx, _, finished := process.GetManager().AddContext(txCtx, fmt.Sprintf("ArchiveRequest[%d]: %s", r.RepoID, r.GetArchiveName())) + ctx, _, finished := process.GetManager().AddContext(ctx, fmt.Sprintf("ArchiveRequest[%d]: %s", r.RepoID, r.GetArchiveName())) defer finished() archiver, err := repo_model.GetRepoArchiver(ctx, r.RepoID, r.Type, r.CommitID) @@ -192,7 +193,7 @@ func doArchive(ctx context.Context, r *ArchiveRequest) (*repo_model.RepoArchiver return nil, err } } - return archiver, committer.Commit() + return archiver, nil } if !errors.Is(err, os.ErrNotExist) { @@ -261,17 +262,7 @@ func doArchive(ctx context.Context, r *ArchiveRequest) (*repo_model.RepoArchiver } } - return archiver, committer.Commit() -} - -// ArchiveRepository satisfies the ArchiveRequest being passed in. Processing -// will occur in a separate goroutine, as this phase may take a while to -// complete. If the archive already exists, ArchiveRepository will not do -// anything. In all cases, the caller should be examining the *ArchiveRequest -// being returned for completion, as it may be different than the one they passed -// in. -func ArchiveRepository(ctx context.Context, request *ArchiveRequest) (*repo_model.RepoArchiver, error) { - return doArchive(ctx, request) + return archiver, nil } var archiverQueue *queue.WorkerPoolQueue[*ArchiveRequest] @@ -281,8 +272,10 @@ func Init(ctx context.Context) error { handler := func(items ...*ArchiveRequest) []*ArchiveRequest { for _, archiveReq := range items { log.Trace("ArchiverData Process: %#v", archiveReq) - if _, err := doArchive(ctx, archiveReq); err != nil { + if archiver, err := doArchive(ctx, archiveReq); err != nil { log.Error("Archive %v failed: %v", archiveReq, err) + } else { + log.Trace("ArchiverData Success: %#v", archiver) } } return nil diff --git a/services/repository/archiver/archiver_test.go b/services/repository/archiver/archiver_test.go index ec6e9dfac3..b3f3ed7bf3 100644 --- a/services/repository/archiver/archiver_test.go +++ b/services/repository/archiver/archiver_test.go @@ -80,13 +80,13 @@ func TestArchive_Basic(t *testing.T) { inFlight[1] = tgzReq inFlight[2] = secondReq - ArchiveRepository(db.DefaultContext, zipReq) - ArchiveRepository(db.DefaultContext, tgzReq) - ArchiveRepository(db.DefaultContext, secondReq) + doArchive(db.DefaultContext, zipReq) + doArchive(db.DefaultContext, tgzReq) + doArchive(db.DefaultContext, secondReq) // Make sure sending an unprocessed request through doesn't affect the queue // count. - ArchiveRepository(db.DefaultContext, zipReq) + doArchive(db.DefaultContext, zipReq) // Sleep two seconds to make sure the queue doesn't change. time.Sleep(2 * time.Second) @@ -101,7 +101,7 @@ func TestArchive_Basic(t *testing.T) { // We still have the other three stalled at completion, waiting to remove // from archiveInProgress. Try to submit this new one before its // predecessor has cleared out of the queue. - ArchiveRepository(db.DefaultContext, zipReq2) + doArchive(db.DefaultContext, zipReq2) // Now we'll submit a request and TimedWaitForCompletion twice, before and // after we release it. We should trigger both the timeout and non-timeout @@ -109,7 +109,7 @@ func TestArchive_Basic(t *testing.T) { timedReq, err := NewRequest(ctx.Repo.Repository.ID, ctx.Repo.GitRepo, secondCommit+".tar.gz") assert.NoError(t, err) assert.NotNil(t, timedReq) - ArchiveRepository(db.DefaultContext, timedReq) + doArchive(db.DefaultContext, timedReq) zipReq2, err = NewRequest(ctx.Repo.Repository.ID, ctx.Repo.GitRepo, firstCommit+".zip") assert.NoError(t, err)