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

Object tagging (#4754) #4999

Merged
merged 3 commits into from
Oct 30, 2023
Merged

Object tagging (#4754) #4999

merged 3 commits into from
Oct 30, 2023

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Oct 26, 2023

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?

Comment on lines 1907 to 1910
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,
Copy link
Contributor Author

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...

/// Supported keys:
/// - `aws_skip_tagging`
/// - `skip_tagging`
SkipTagging,
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 don't feel strongly about this name

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe DisableTagging

Copy link
Contributor

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 :)

Copy link
Contributor

@thinkharderdev thinkharderdev left a 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.

/// Supported keys:
/// - `aws_skip_tagging`
/// - `skip_tagging`
SkipTagging,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe DisableTagging

/// Supported keys:
/// - `aws_skip_tagging`
/// - `skip_tagging`
SkipTagging,
Copy link
Contributor

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 :)

@tustvold tustvold merged commit 11b2f5f into apache:master Oct 30, 2023
13 checks passed
@tustvold tustvold added the api-change Changes to the arrow API label Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support User-Defined Object Metadata
2 participants