Skip to content

Comments

feat(services/cos): Added user metadata support for cos service#5510

Merged
Xuanwo merged 14 commits intoapache:mainfrom
geetanshjuneja:cos
Jan 13, 2025
Merged

feat(services/cos): Added user metadata support for cos service#5510
Xuanwo merged 14 commits intoapache:mainfrom
geetanshjuneja:cos

Conversation

@geetanshjuneja
Copy link
Contributor

Which issue does this PR close?

Closes cos service task for #4842 .

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@geetanshjuneja geetanshjuneja requested a review from Xuanwo as a code owner January 5, 2025 15:52
@github-actions github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Jan 5, 2025
@geetanshjuneja
Copy link
Contributor Author

@Xuanwo can you plz look into this PR.

if let Some(user_metadata) = args.user_metadata() {
for (key, value) in user_metadata {
// before insert user defined metadata header, add prefix to the header name
if !self.check_user_metadata_key(key) {
Copy link
Member

Choose a reason for hiding this comment

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

Hi, I'm not sure if it's a good idea to check user's metadata key. Let's skip it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

/// # Notes
///
/// before return the user defined metadata, we'll strip the user_metadata_prefix from the key
pub fn parse_metadata(&self, path: &str, headers: &HeaderMap) -> Result<Metadata> {
Copy link
Member

Choose a reason for hiding this comment

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

I think this function might be slightly overengineered.

Copy link
Contributor Author

@geetanshjuneja geetanshjuneja Jan 6, 2025

Choose a reason for hiding this comment

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

What do you mean by overengineered? What do I need to change in this fn?

Copy link
Member

Choose a reason for hiding this comment

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

Hi, I would like to remove this function and expand it instead.

@geetanshjuneja
Copy link
Contributor Author

@Xuanwo I have made the changes that you suggested.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @geetanshjuneja for working on this!

@Xuanwo Xuanwo merged commit 5ae1a76 into apache:main Jan 13, 2025
93 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants