-
Notifications
You must be signed in to change notification settings - Fork 824
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
Object tagging (#4754) #4999
Object tagging (#4754) #4999
Conversation
object_store/src/lib.rs
Outdated
pub(crate) async fn tagging<F, Fut>(storage: &dyn ObjectStore, get_tags: F) | ||
where | ||
F: Fn(Path) -> Fut + Send + Sync, | ||
Fut: Future<Output = Result<reqwest::Response>> + Send, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly working out the correct combination of lifetimes and send and sync took longer than anything else in this PR... Sometimes I really dislike async...
object_store/src/aws/builder.rs
Outdated
/// Supported keys: | ||
/// - `aws_skip_tagging` | ||
/// - `skip_tagging` | ||
SkipTagging, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about this name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe DisableTagging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I don't feel strongly either :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'm not very familiar with Azure so can't say whether that is done correctly or not but the S3 implementation looks good.
object_store/src/aws/builder.rs
Outdated
/// Supported keys: | ||
/// - `aws_skip_tagging` | ||
/// - `skip_tagging` | ||
SkipTagging, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe DisableTagging
object_store/src/aws/builder.rs
Outdated
/// Supported keys: | ||
/// - `aws_skip_tagging` | ||
/// - `skip_tagging` | ||
SkipTagging, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I don't feel strongly either :)
Which issue does this PR close?
Closes #4754.
Rationale for this change
Some stores support object tagging and this is important for existing workloads. Unfortunately support for this is not supported at all in some cases, e.g. GCS, and inconsistently supported in others, e.g. Azure and AWS-compatible stores.
What changes are included in this PR?
Are there any user-facing changes?