Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

keep stat() and list() lastModified upto same resolution #1638

Merged
merged 1 commit into from Apr 18, 2022

Conversation

harshavardhana
Copy link
Member

fixes #1632

@jhwz
Copy link

jhwz commented Apr 16, 2022

Quick note (I'm the OP of #1632) I found in my tests that to keep that same behaviour I needed to use:

LastModified.Add(-time.Millisecond * 500).Round(time.Second)

instead of

LastModified.Round(time.Second)

to floor the time, not round it. Not sure if that will solve the tests not completing

@harshavardhana
Copy link
Member Author

No it shouldn't be Round instead it should be Truncate() @jhwz

@jhwz
Copy link

jhwz commented Apr 16, 2022

No it shouldn't be Round instead it should be Truncate() @jhwz

Ah right, that works too

@vadmeste
Copy link
Member

@harshavardhana can't we make Stat() to show more precise time instead of truncating to second ?

I don't think it will break anything since is this a go library and time.Parse(time.RFC3339, "2022-04-16T13:37:01.511096Z") works just fine (time.RFC3339 without Nano)

I think it is just better to keep time precision here rather than losing it (not sure what is the effect for bucket replication here as well)

@harshavardhana
Copy link
Member Author

harshavardhana commented Apr 16, 2022

@harshavardhana can't we make Stat() to show more precise time instead of truncating to second ?

I don't think it will break anything since is this a go library and time.Parse(time.RFC3339, "2022-04-16T13:37:01.511096Z") works just fine (time.RFC3339 without Nano)

I think it is just better to keep time precision here rather than losing it (not sure what is the effect for bucket replication here as well)

The problem is there is no time precision allowed in AWS S3. It's only up to second precision @vadmeste

That's why millisecond values don't make sense.

Bucket replication doesn't rely on these values, it relies on internal values to obtain the right precision.

@harshavardhana
Copy link
Member Author

Tested this with AWS S3 @vadmeste the ListObjects doesn't have the millisecond resolution

AWS_REGION=us-east-1 ./example 
2022/04/17 13:34:42 first page results:
Name:           hosts
ListObjectsV2 -> Last modified:  2022-04-17 20:33:17 +0000 UTC
Size:           303
Storage class:  STANDARD

HeadObject() Last Modified:  2022-04-17 20:33:17 +0000 UTC
package main

import (
	"context"
	"fmt"
	"log"

	"github.com/aws/aws-sdk-go-v2/aws"
	"github.com/aws/aws-sdk-go-v2/config"
	"github.com/aws/aws-sdk-go-v2/service/s3"
)

func main() {
	// Load the Shared AWS Configuration (~/.aws/config)
	// cfg, err := config.LoadDefaultConfig(context.TODO(), config.WithSharedConfigProfile("minio"), config.WithEndpointResolverWithOptions(customResolver))
	cfg, err := config.LoadDefaultConfig(context.TODO(), config.WithSharedConfigProfile("s3"))
	if err != nil {
		log.Fatal(err)
	}
	
	// Create an Amazon S3 service client
	client := s3.NewFromConfig(cfg, func(opts *s3.Options) {
		opts.UsePathStyle = true
	})

	// Get the first page of results for ListObjectsV2 for a bucket
	output, err := client.ListObjectsV2(context.TODO(), &s3.ListObjectsV2Input{
		Bucket: aws.String("vadmeste"),
		Prefix: aws.String("hosts"),
	})
	if err != nil {
		log.Fatal(err)
	}

	log.Println("first page results:")
	for _, item := range output.Contents {
		fmt.Println("Name:          ", *item.Key)
		fmt.Println("ListObjectsV2 -> Last modified: ", *item.LastModified)
		fmt.Println("Size:          ", item.Size)
		fmt.Println("Storage class: ", item.StorageClass)
		fmt.Println("")
	}

	houtput, err := client.HeadObject(context.TODO(), &s3.HeadObjectInput{
		Bucket: aws.String("vadmeste"),
		Key:    aws.String("hosts"),
	})

	fmt.Println("HeadObject() Last Modified: ", *houtput.LastModified)
}

So this PR is valid and we should also perhaps remove millisecond resolution from the MinIO server.

// cc @kannappanr @klauspost

@vadmeste
Copy link
Member

/cc @poornas does this change affect bucket replication ?

@poornas
Copy link
Contributor

poornas commented Apr 18, 2022

/cc @poornas does this change affect bucket replication
No, it will not

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@harshavardhana harshavardhana merged commit fbcac8c into minio:master Apr 18, 2022
@harshavardhana harshavardhana deleted the round-list branch April 18, 2022 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent LastModified field
4 participants