Skip to content

Commit

Permalink
[CR] Sleep only on S3 in multipart test, and fix some comments
Browse files Browse the repository at this point in the history
  • Loading branch information
arielshaqed committed Oct 30, 2024
1 parent a732383 commit 5dd27ca
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 10 deletions.
9 changes: 6 additions & 3 deletions esti/multipart_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/stretchr/testify/require"
"github.com/thanhpk/randstr"
"github.com/treeverse/lakefs/pkg/api/apigen"
"github.com/treeverse/lakefs/pkg/block"
"github.com/treeverse/lakefs/pkg/logging"
)

Expand Down Expand Up @@ -53,9 +54,11 @@ func TestMultipartUpload(t *testing.T) {

completedParts := uploadMultipartParts(t, ctx, logger, resp, parts, 0)

// On S3 the object should have Last-Modified time at around time of MPU creation.
// Ensure lakeFS fails the test if it fakes it by using the current time.
time.Sleep(2 * timeResolution)
if isBlockstoreType(block.BlockstoreTypeS3) == nil {
// Object should have Last-Modified time at around time of MPU creation. Ensure
// lakeFS fails the test if it fakes it by using the current time.
time.Sleep(2 * timeResolution)
}

completeResponse, err := uploadMultipartComplete(ctx, svc, resp, completedParts)
require.NoError(t, err, "failed to complete multipart upload")
Expand Down
15 changes: 12 additions & 3 deletions esti/system_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -337,11 +337,20 @@ func listRepositories(t *testing.T, ctx context.Context) []apigen.Repository {
return listedRepos
}

// isBlockstoreType returns nil if the blockstore type is one of requiredTypes, or the actual
// type of the blockstore.
func isBlockstoreType(requiredTypes ...string) *string {
blockstoreType := viper.GetString(config.BlockstoreTypeKey)
if slices.Contains(requiredTypes, blockstoreType) {
return nil
}
return &blockstoreType
}

// requireBlockstoreType Skips test if blockstore type doesn't match the required type
func requireBlockstoreType(t testing.TB, requiredTypes ...string) {
blockstoreType := viper.GetString(config.BlockstoreTypeKey)
if !slices.Contains(requiredTypes, blockstoreType) {
t.Skipf("Required blockstore types: %v, got: %s", requiredTypes, blockstoreType)
if blockstoreType := isBlockstoreType(requiredTypes...); blockstoreType != nil {
t.Skipf("Required blockstore types: %v, got: %s", requiredTypes, *blockstoreType)
}
}

Expand Down
6 changes: 3 additions & 3 deletions pkg/api/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -384,10 +384,10 @@ func (c *Controller) CompletePresignMultipartUpload(w http.ResponseWriter, r *ht

writeTime := time.Now()
checksum := httputil.StripQuotesAndSpaces(mpuResp.ETag)
// Anything else can be _really_ wrong when the storage layer assigns the time of MPU
// creation. For instance, the S3 block adapter makes sure to return an MTime from
// headObject to ensure that we do have a time here.
if mpuResp.MTime != nil {
// Anything else can be _really_ wrong when the storage layer assigns the time
// of MPU creation. For instance, the S3 block adapter takes sure to return an
// MTime from headObject to ensure that we do have a time here.
writeTime = *mpuResp.MTime
}
entryBuilder := catalog.NewDBEntryBuilder().
Expand Down
1 change: 0 additions & 1 deletion pkg/gateway/operations/putobject.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,6 @@ func handlePut(w http.ResponseWriter, req *http.Request, o *PathOperation) {
// write metadata
metadata := amzMetaAsMetadata(req)
contentType := req.Header.Get("Content-Type")
// BUG(ariels): Read MTime from upload!
err = o.finishUpload(req, nil, blob.Checksum, blob.PhysicalAddress, blob.Size, true, metadata, contentType)
if errors.Is(err, graveler.ErrWriteToProtectedBranch) {
_ = o.EncodeError(w, req, err, gatewayErrors.Codes.ToAPIErr(gatewayErrors.ErrWriteToProtectedBranch))
Expand Down

0 comments on commit 5dd27ca

Please sign in to comment.