blob: add modification time to driver.ObjectAttrs #240

benhinchley · 2018-07-26T01:16:18Z

At Sajari we have been doing something similar to blob with our own storage pkg for quite some time now.

I started working on migrating our storage pkg to use go-cloud/blob underneath, and noticed that modification time was not a part of the driver.ObjectAttrs struct. This is something that we use internally and is a part of our storage.File struct.

This adds ModTime to driver.ObjectAttrs and adds it to the current driver implementations.

googlebot · 2018-07-26T01:16:20Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

benhinchley · 2018-07-26T01:20:32Z

I signed it!

googlebot · 2018-07-26T01:20:34Z

CLAs look good, thanks!

zombiezen

Thank you for your contribution! Looks good overall, just had a few comments.

@shantuo, could you add integration tests for GCS and S3?

zombiezen · 2018-07-26T19:56:23Z

blob/gcsblob/gcsblob.go

@@ -65,11 +68,18 @@ func (r *reader) Attrs() *driver.ObjectAttrs {
 func (b *bucket) NewRangeReader(ctx context.Context, key string, offset, length int64) (driver.Reader, error) {
 	bkt := b.client.Bucket(b.name)
 	obj := bkt.Object(key)
+	attrs, err := obj.Attrs(ctx)


It's unfortunate that this requires a second network call, but it was bound to happen eventually.

My one concern is that there is a race here: Attrs could be for a different version than the reader is for. In this case, there's not that much cause for concern, but I'd like to add the constraint now so that as the code evolves, we don't hit the issue later.

One alternative is to not call obj.Attrs inside NewRangeReader, instead, call it when user explicitly asks for it the first time. At that point we can check whether this field is non-zero and decide to make the call. WDYT?

The updated field is included in the object but not carried by the client library we use here. I suggest removing the implementation here for GCS and keep those for S3 and file, add comments to the blob package saying that this ModTime field is optional. We'll try to have it supported by the client library first and add it back here.

zombiezen · 2018-07-26T19:59:36Z

blob/driver/driver.go

@@ -66,6 +67,8 @@ type ObjectAttrs struct {
 	Size int64
 	// ContentType is the MIME type of the blob object. It must not be empty.
 	ContentType string
+	// ModTime is the modified time of the blob object.


I can imagine circumstances where implementations might not have this information. Let's add a comment to the effect that ModTime might be the zero time if unknown.

zombiezen · 2018-07-26T20:04:20Z

blob/fileblob/fileblob.go

@@ -148,6 +152,7 @@ func (r reader) Attrs() *driver.ObjectAttrs {
 	return &driver.ObjectAttrs{
 		Size:        r.size,
 		ContentType: r.xa.ContentType,
+		ModTime:     r.modTime,


Please also add tests for this new functionality for fileblob.

zombiezen · 2018-08-06T20:39:59Z

@benhinchley Is this ready for @shantuo's review?

zombiezen

(Approving to unblock further review, but I'm going to let @shantuo review.)

benhinchley · 2018-08-07T00:08:01Z

@zombiezen yep changes you suggested have been made

shantuo

Thank you for the PR, it generally look good. I just left a comment about the decision on the GCS implementation.

shantuo · 2018-08-07T21:10:01Z

blob/gcsblob/gcsblob.go

@@ -65,11 +68,18 @@ func (r *reader) Attrs() *driver.ObjectAttrs {
 func (b *bucket) NewRangeReader(ctx context.Context, key string, offset, length int64) (driver.Reader, error) {
 	bkt := b.client.Bucket(b.name)
 	obj := bkt.Object(key)
+	attrs, err := obj.Attrs(ctx)


The updated field is included in the object but not carried by the client library we use here. I suggest removing the implementation here for GCS and keep those for S3 and file, add comments to the blob package saying that this ModTime field is optional. We'll try to have it supported by the client library first and add it back here.

ModTime has the potenital to be time.Time zero value if a cloud storage impl doesn't provide this information

benhinchley · 2018-08-09T00:06:08Z

@shantuo I've update the gcsblob impl and added the comment to blob.ModTime() about this possibly returning time.Time zero value

benhinchley mentioned this pull request Jul 26, 2018

Investigate building connectors for go-cloud sajari/storage#4

Open

benhinchley changed the title ~~blob(WIP): add modification time to driver.ObjectAttrs~~ blob: add modification time to driver.ObjectAttrs Jul 26, 2018

zombiezen added the enhancement New feature or request label Jul 26, 2018

zombiezen suggested changes Jul 26, 2018

View reviewed changes

zombiezen requested review from shantuo and zombiezen and removed request for zombiezen August 6, 2018 20:40

zombiezen approved these changes Aug 6, 2018

View reviewed changes

googlebot added the cla: yes Google CLA has been signed! label Aug 7, 2018

shantuo reviewed Aug 7, 2018

View reviewed changes

Benjamin Hinchley added 7 commits August 9, 2018 10:01

blob/driver: add ModTime to ObjectAttrs

77ee19e

blob/gcsblob: add modTime to reader

58b1d4b

blob/fileblob: add modTime to reader

fa8428a

blob/s3blob: add modTime to reader

494ea51

blob/driver: update comment on ModTime

83c61c5

ModTime has the potenital to be time.Time zero value if a cloud storage impl doesn't provide this information

blob/fileblob: add test for ModTime

c0aa855

blob: remove ModTime from gcsblob, add comment to blob.ModTime

7203031

shantuo approved these changes Aug 9, 2018

View reviewed changes

Merge branch 'master' into add-modtime-to-blob

71121b7

shantuo merged commit cddd637 into google:master Aug 9, 2018

benhinchley deleted the add-modtime-to-blob branch August 10, 2018 02:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blob: add modification time to driver.ObjectAttrs #240

blob: add modification time to driver.ObjectAttrs #240

benhinchley commented Jul 26, 2018 •

edited

Loading

googlebot commented Jul 26, 2018

benhinchley commented Jul 26, 2018

googlebot commented Jul 26, 2018

zombiezen left a comment

zombiezen Jul 26, 2018

shantuo Jul 27, 2018

shantuo Aug 7, 2018

zombiezen Jul 26, 2018

zombiezen Jul 26, 2018

zombiezen commented Aug 6, 2018

zombiezen left a comment

benhinchley commented Aug 7, 2018

shantuo left a comment

shantuo Aug 7, 2018

benhinchley commented Aug 9, 2018

blob: add modification time to driver.ObjectAttrs #240

blob: add modification time to driver.ObjectAttrs #240

Conversation

benhinchley commented Jul 26, 2018 • edited Loading

googlebot commented Jul 26, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

benhinchley commented Jul 26, 2018

googlebot commented Jul 26, 2018

zombiezen left a comment

Choose a reason for hiding this comment

zombiezen Jul 26, 2018

Choose a reason for hiding this comment

shantuo Jul 27, 2018

Choose a reason for hiding this comment

shantuo Aug 7, 2018

Choose a reason for hiding this comment

zombiezen Jul 26, 2018

Choose a reason for hiding this comment

zombiezen Jul 26, 2018

Choose a reason for hiding this comment

zombiezen commented Aug 6, 2018

zombiezen left a comment

Choose a reason for hiding this comment

benhinchley commented Aug 7, 2018

shantuo left a comment

Choose a reason for hiding this comment

shantuo Aug 7, 2018

Choose a reason for hiding this comment

benhinchley commented Aug 9, 2018

benhinchley commented Jul 26, 2018 •

edited

Loading