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

Question: what is the migration path for BucketAlreadyOwnedByYou and BucketAlreadyExists errors? #1051

Closed
ell1e opened this issue Jan 2, 2021 · 7 comments

Comments

@ell1e
Copy link

ell1e commented Jan 2, 2021

What is the migration path for BucketAlreadyOwnedByYou and BucketAlreadyExists errors? These imports just seem to be gone and I can't find a single relevant result on Google on what happened with them. Maybe it would be useful to add a note to minio/error.py at the top in the form of a code comment that gives some info on where they went, for people like me opening up the file and trying to figure out what happened.

@balamurugana
Copy link
Member

Both BucketAlreadyOwnedByYou/BucketAlreadyExists errors are valid as per S3 specification. We should check for both error to find whether the bucket already exists.

minio/error.py does not handle anything specific to these errors. Please reopen the issue by provide what exactly you are expecting and what is missing now.

@ell1e
Copy link
Author

ell1e commented Jan 2, 2021

@balamurugana minio/error.py used to define these names, now it doesn't. I get an ImportError when running my code and I'm not sure what I need to use now to handle those errors. Sorry for not being clear.

@balamurugana
Copy link
Member

@ell1e The latest release v7 is a breaking release. Please refer examples and API guide

@ell1e
Copy link
Author

ell1e commented Jan 2, 2021

I did, but this is really tedious to go through to find the new variant of what I am using if I don't know what name I am looking for. This is why a migration note would be great, instead of just dropping the new API with no info about how it relates to the old one. Is there one?

@ell1e
Copy link
Author

ell1e commented Jan 2, 2021

One example: this uses make_bucket as I do: https://github.com/minio/minio-py/blob/master/examples/make_bucket.py

... but it doesn't handle any errors, while my code does. But what does that mean? Am I not supposed to handle BucketAlreadyExists for make_bucket anymore, or just differently? Or does the new make_bucket not throw any errors at all anymore? If there was some summary about the biggest changes like e.g. what I'm in general supposed to do about all the error names that are gone like BucketAlreadyExists (has it been merged into S3Error by any chance?) that would be useful. Also, maybe make_bucket.py should be updated with error handling to show how that is supposed to be done, since I assume it can throw an error still.

Edit: just checked the API docs, they don't seem to have any specific info on what errors make_bucket is now expected to throw either. I assume you probably want people to just check the S3 documentation instead. Maybe it would be worth linking it for each of the respective functions or something? Like, just some way so people can get an easier quick idea of what the possible corner cases and error states are that may need to be handled for an operation.

@balamurugana
Copy link
Member

balamurugana commented Jan 2, 2021

How about below code?

if not client.bucket_exists("my-bucket"):
    client.make_bucket("my-bucket")

To specific to make bucket, it is usually done only one in life time. When error is concerned with APIs, there is nothing specific unless user wants to deal with S3 API.

minio-py v7 comes with lot of additions, improvements and correctness + detailed API guide and examples. Majority of older APIs do not break because of new additional parameters. However error handling is surely incompatible because previous versions did not provide right way of handling errors.

You could refer README for basic S3Error handling. S3Error has code property contain S3 error

@ell1e
Copy link
Author

ell1e commented Jan 2, 2021

That code looks like a race condition waiting to happen, and therefore useless in production code. You really shouldn't suggest that to anyone, IMHO. (My code is written under the assumption that the app backend is stateless, hence this could be run in multiple backend containers in parallel and blow up. I would assume most minio-using backends would be written with this assumption, otherwise I could just use a local folder for file storage and not S3/minio.)

The only sane way is obviously to check S3Error, but only for the specific known error condition that it already exists. That was easily possible with BucketAlreadyExists, but now there's just opaque S3Error with no example code anywhere of how to check for a specific S3 error instead of just any. The README is kind of useless here as well because it doesn't check code. What is that even, a string?

A migration guide specifically for sane error handling, and updating your examples to handle errors would really help a lot. Because this was previously pretty obvious on how to do it, but now it's not - not only for people migrating but for newcomers as well.

Edit: I apologize, my wording was unnecessarily negative. It is nice that you're responding with some ideas for sure, it is helpful for example to know that code is what I am looking for, no matter the exact use. That already helps a lot.

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

No branches or pull requests

2 participants