Skip to content

Conversation

@kesselb
Copy link
Contributor

@kesselb kesselb commented Oct 1, 2018

I've noticed when looking into #6954 that there are many unneccessary headObject requests.

According to https://docs.aws.amazon.com/AmazonS3/latest/API/v2-RESTBucketGET.html#v2-RESTBucketGET-responses-examples Size, LastModified and Etag returned with listObjects.

image

When calling stat for a file the required information are size and last modified. Reusing this information from listObjects may save some api calls.

@kesselb kesselb added this to the Nextcloud 15 milestone Oct 1, 2018
@kesselb kesselb added the 3. to review Waiting for reviews label Oct 1, 2018
@MorrisJobke
Copy link
Member

Code changes look good, but I would like to wait for the feedback of @icewind1991 here.

Copy link
Member

@icewind1991 icewind1991 left a comment

Choose a reason for hiding this comment

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

Looks good otherwise


$result = $this->headObject($path);
if (isset($result['ContentLength'])) {
return $result['ContentLength'];
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice if this would fill in the filesCache from the head response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been thinking about that but headObject has already some internal cache. I think this would just add additional complexity. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough

@icewind1991 icewind1991 dismissed their stale review October 8, 2018 16:14

already cached

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code makes sense 👍

@MorrisJobke MorrisJobke merged commit 2a9e006 into master Oct 8, 2018
@MorrisJobke MorrisJobke deleted the feature/noid/save-s3-requests branch October 8, 2018 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants