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

gcsblob: reader.ModTime() returns zero but the API returns it #314

Closed
evanj opened this Issue Aug 12, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@evanj

evanj commented Aug 12, 2018

The reader.modTime field is never updated: https://github.com/google/go-cloud/blob/master/blob/gcsblob/gcsblob.go#L54. This is sort of okay, since the documentation for ModTime() states: "This is optional and will be time.Time zero value if unknown" . However, the underlying HTTP response includes the modification time in the Last-Modified header. Unfortunately, cloud.google.com/go/storage does not parse it. Perhaps that library should do so, or at least this library should explicitly call ObjectHandle.Attrs() if NewRangeReader length == 0, so it is possible to get the modification time in some way.

My use case: I am synchronizing files from one source to another, and it can be useful to check the modification time to determine if something is likely to need to be copied. This works fine for s3 but not for GCS.

@evanj evanj changed the title from gcpblob: reader.ModTime() always returns zero time but the API returns it to gcpblob: reader.ModTime() returns zero but the API returns it Aug 12, 2018

@shantuo shantuo changed the title from gcpblob: reader.ModTime() returns zero but the API returns it to gcsblob: reader.ModTime() returns zero but the API returns it Aug 13, 2018

@shantuo

This comment has been minimized.

Contributor

shantuo commented Aug 13, 2018

We chose not to call ObjectHandle.Attrs to save the network request as the Last-Modified is returned from the HTTP response, instead, we'd like to fix the Go client library of GCS to fix this. We filed googleapis/google-cloud-go#1091 to track.

@evanj

This comment has been minimized.

evanj commented Aug 14, 2018

Thanks! I think fixing google-cloud-go is the "best" fix, so if that is easy that is great. However to clarify my suggestion if changing that library is hard: when length == 0, call ObjectHandle.Attrs() instead of calling ObjectHandle.NewRangeReader(). This would be similar to s3blob.NewRangeReader which returns a special newMetadataReader result in this case.

@vangent vangent self-assigned this Aug 14, 2018

@vangent vangent added in progress and removed blocked labels Aug 14, 2018

@vangent

This comment has been minimized.

Contributor

vangent commented Aug 14, 2018

Sounds reasonable to me. We'll get ModTime for 0-length reads via Attrs; we still won't get it for >0-length reads, pending the fix for googleapis/google-cloud-go#1091.

@vangent vangent added this to the Sprint 15 milestone Aug 14, 2018

vangent added a commit to vangent/go-cloud that referenced this issue Aug 15, 2018

vangent added a commit that referenced this issue Aug 16, 2018

gcsblob: Fetch ModTIme via ObjectHandle.Attrs() for 0-length reads. (#…
…330)

We still don't get ModTime for non-0-length reads, pending
googleapis/google-cloud-go#1091.

Issue #314.

@vangent vangent modified the milestones: Sprint 15, Unplanned Aug 16, 2018

@vangent vangent removed the blocked label Aug 16, 2018

@vangent vangent modified the milestones: Unplanned, Sprint 15 Aug 16, 2018

@vangent

This comment has been minimized.

Contributor

vangent commented Aug 16, 2018

I'm marking this fixed; there's still the same issue for non-zero length reads, but it's blocked by googleapis/google-cloud-go#1091.

@vangent

This comment has been minimized.

Contributor

vangent commented Aug 16, 2018

Issue #335 is the new one I just filed.

@evanj

This comment has been minimized.

evanj commented Aug 16, 2018

Wow thanks! This looks great; time to re-test my app ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment