Removing Exists() check from S3 getters

[linzhp]

What is the problem I am trying to address?

Athens first checks the existence of a module/version before:

1. Reading mod, info or zip file
2. Deleting a module
3. Writing a module/version fetched from VCS

Some of these checks are not necessary, because

• we can know that from the returned error of 1
• For 2, S3 doesn't return errors when we tries to delete an non-existing blob, but users may not need to know that either. In fact, I don't see where Athens is deleting a blob, any pointers?

Removing these checks will improve the atomicity and performance of the operations, which would also address the performance issue described in #1840

3 is the only case when we need to check for existence to avoid duplicate uploads, but we can potentially make HeadObject calls in the Uploader, which is called concurrently. It would make the check more granular: instead of uploading all 3 files again when one file is missing, we can upload only the missing file.

How is the fix applied?

• removing the Exists() check from getter.go and instead read the return error from GetObjectWithContext to determine whether it is not found.

If this approach is OK, I can apply similar changes to other storage clients.

@ngshiheng can you test this?

@marwan-at-work @r-ashish What do you think?

[manugupt1]

@linzhp @ngshiheng For performance issues: I think we can replace ListObjects call in S3 with HeadObject (as suggested; sorry did not read all of it) for all the 3 and it should significantly speed things up. If this works, we can keep the exists check around.

The key should be: config.PackageVersionedName(module, version, "info") when making the head call. wdyt?

[manugupt1]

I think Delete is to help Delete specific modules. It is a path

1. /{module:.+}/@v/{version}.delete

[linzhp]

@linzhp @ngshiheng For performance issues: I think we can replace ListObjects call in S3 with HeadObject (as suggested; sorry did not read all of it) for all the 3 and it should significantly speed things up. If this works, we can keep the exists check around.

The key should be: config.PackageVersionedName(module, version, "info") when making the head call. wdyt?

Yes, this is a good option too. I can do it in a separate PR.

[ngshiheng]

@linzhp @ngshiheng For performance issues: I think we can replace ListObjects call in S3 with HeadObject (as suggested; sorry did not read all of it) for all the 3 and it should significantly speed things up. If this works, we can keep the exists check around.

The key should be: config.PackageVersionedName(module, version, "info") when making the head call. wdyt?

(I replied at the other PR going to post it here again for reference) I think HeadObject would work here.

However, my main concern is that we would be making 2 additional requests/module (3 in total for .mod, .zip and .info files) using HeadObject if my understanding is correct (AFAIK there's no way we can batch the 3 calls using HeadObject, unfortunately).

Perhaps, the ListObjectsV2 operation may be more efficient since it allows us to retrieve multiple objects in a single API call.

wdyt?

edit: just to put it out there, the HEAD requests limit on S3 is currently at 5,500 req/s (reference).

[linzhp]

With this PR, the Exists() call would be pretty rare: it only happens before writing modules to S3. So 5500 req/s means Athens is allowed to write 1833 modules per second.

[manugupt1]

You can’t predict, if this is a concern the S3 client can have some back offs. I would not worry though. In case, we are throttled, we can always serialize these requests

[linzhp]

With #1844, I think this is still a good improvement to getters. @marwan-at-work Can you take a look too?

[marwan-at-work]

LGTM

[manugupt1]

I am not sure though! Can we hold on to merging this please?

[marwan-at-work]

@manugupt1 What is your concern here? You are more familiar with S3 and AWS than I am so I'm happy to hear your thoughts

[manugupt1]

I am not sure if we should download all the bytes for info, mod and version with a GET call. It’s just that the bytes transferred will introduce latency as well as cost. I maybe wrong but it seems like an over optimization to remove Exists now.

[marwan-at-work]

I am not sure if we should download all the bytes for info, mod and version with a GET call. It’s just that the bytes transferred will introduce latency as well as cost.

From the diff of this PR, all it's doing is that it's not calling ".Check()" but instead using the 404 from the GET call as a way to check if something does not exist. On the flip side, if a module does exist then we just proceed to serving like as if Check() return true. Am I missing something?

Thanks!

[manugupt1]

You are correct! I need sometime to think and understand. Let me get back! I am still not sure though

[manugupt1]

@marwan-at-work @linzhp Sorry! I think this PR looks good to me. I think it is good to go. I did not read the commit properly as I was going through my phone.

[linzhp]

No worries. Thanks for the quick review!