Skip to content

Docs: Add S3 delete tagging docs #4443

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.

Sign up for GitHub

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

Jump to bottom
Merged
jackye1995 merged 1 commit into from
May 24, 2022
Merged

Docs: Add S3 delete tagging docs #4443

jackye1995 merged 1 commit into from
May 24, 2022
+22 −2

Conversation

@rajarshisarkar
Copy link
Contributor

@rajarshisarkar rajarshisarkar commented Mar 31, 2022

@github-actions github-actions bot added the docs label Mar 31, 2022
@rajarshisarkar rajarshisarkar force-pushed the s3-delete-tagging-doc branch from 37cc133 to b3c0a10 Compare March 31, 2022 04:56
singhpk234
singhpk234 reviewed Mar 31, 2022
View reviewed changes
@rajarshisarkar rajarshisarkar force-pushed the s3-delete-tagging-doc branch from b3c0a10 to 0284519 Compare April 4, 2022 10:28
singhpk234
singhpk234 reviewed Apr 5, 2022
View reviewed changes
Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

Thanks @rajarshisarkar for this change !!!

@rajarshisarkar rajarshisarkar force-pushed the s3-delete-tagging-doc branch 3 times, most recently from d42e4f2 to 6ec115a Compare April 11, 2022 10:18
singhpk234
singhpk234 reviewed Apr 11, 2022
View reviewed changes
singhpk234
singhpk234 approved these changes Apr 11, 2022
View reviewed changes
Copy link
Contributor

@singhpk234 singhpk234 left a comment

Choose a reason for hiding this comment

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

LGTM @rajarshisarkar !!!, just a question on soft-delete

@rajarshisarkar rajarshisarkar force-pushed the s3-delete-tagging-doc branch from 6ec115a to 64adb47 Compare April 11, 2022 12:07
amogh-jahagirdar
amogh-jahagirdar reviewed Apr 11, 2022
View reviewed changes
@rajarshisarkar rajarshisarkar force-pushed the s3-delete-tagging-doc branch from 64adb47 to 93420b4 Compare April 12, 2022 09:36
jackye1995
jackye1995 reviewed Apr 26, 2022
View reviewed changes
jackye1995
jackye1995 reviewed Apr 26, 2022
View reviewed changes
docs/integrations/aws.md Outdated
### S3 Delete

When the catalog property `s3.delete-enabled` is set to `false`, the objects are not deleted from S3.
This is set to `true` by default.
Copy link
Contributor

@jackye1995 jackye1995 Apr 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

we should link to the next section with something in the line of "This is expected to be used in combination with s3 delete tagging", so objects are tagged and removed using S3 lifecycle policy. See xxx (link to next section) for more details".

Copy link
Contributor

@jackye1995 jackye1995 Apr 26, 2022

Choose a reason for hiding this comment

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

Actually, given that the delete-enabled is only relevant under the context of tagging, would it make more sense to just mention this in the tagging section below, instead of having its own section?

Copy link
Contributor Author

@rajarshisarkar rajarshisarkar Apr 26, 2022

Choose a reason for hiding this comment

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

Makes sense to me. I have made the changes.

@rajarshisarkar rajarshisarkar force-pushed the s3-delete-tagging-doc branch 2 times, most recently from 712e10b to 88af363 Compare April 26, 2022 08:29
@rajarshisarkar rajarshisarkar force-pushed the s3-delete-tagging-doc branch from 88af363 to 4d599b0 Compare April 26, 2022 08:57
@samredai
Copy link
Contributor

samredai commented May 4, 2022

This isn't a comment about this PR specifically but the content in the aws.md file feels like a collection of many unrelated things under the AWS umbrella. Maybe we should start thinking about how to break this up, such as moving the S3 docs to a FileIO section, move the Spark material somewhere in the Spark section, etc.

@rajarshisarkar
Copy link
Contributor Author

rajarshisarkar commented May 4, 2022

Thanks, for the inputs @samredai !

@jackye1995 Please let me know your thoughts on the scope of improvement for aws.md. We can create tasks accordingly.

jackye1995
jackye1995 approved these changes May 24, 2022
View reviewed changes
Copy link
Contributor

@jackye1995 jackye1995 left a comment

Choose a reason for hiding this comment

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

Thanks for the work!

@jackye1995 jackye1995 merged commit 77b1e9b into apache:master May 24, 2022
@samredai
Copy link
Contributor

samredai commented May 24, 2022

I have a PR for consolidating configurations, it might be good to add these properties there like spark.sql.catalog.<catalog>.s3.delete.tags.<key>: #4801

@rajarshisarkar
Copy link
Contributor Author

rajarshisarkar commented May 25, 2022

@samredai Yeah, makes sense to me.

@samredai
Copy link
Contributor

samredai commented May 26, 2022 •
edited
Loading

Added to the configurations page PR: https://github.com/apache/iceberg/pull/4801/files#diff-947e999f40ea405c702496fc987ab2c322222d2488680be54ddfa3447c3569faR155-R158

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

Reviewers

@amogh-jahagirdar amogh-jahagirdar amogh-jahagirdar left review comments

@jackye1995 jackye1995 jackye1995 approved these changes

@singhpk234 singhpk234 singhpk234 approved these changes

Assignees

No one assigned

Labels

docs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rajarshisarkar @samredai @jackye1995 @singhpk234 @amogh-jahagirdar