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

fix(services/oss): Fix oss batch max operations #2414

Merged
merged 4 commits into from
Jun 5, 2023

Conversation

A-Stupid-Sun
Copy link
Contributor

related issue #2228
This pr add new batch_max_operations config for oss service and use default value (1000 )if not set.

@A-Stupid-Sun A-Stupid-Sun changed the title Fix oss batch max operations fix(services/oss): Fix oss batch max operations Jun 4, 2023
@Xuanwo
Copy link
Member

Xuanwo commented Jun 5, 2023

Hi, Thanks for the PR! But OSS doesn't need this setting. Maybe you can contact with @manulpatel at PR #2354 to see if you can continue his work?

@Xuanwo
Copy link
Member

Xuanwo commented Jun 5, 2023

But OSS doesn't need this setting.

Since S3 has different implementations such as AWS S3, R2, etc., we require a configuration value for batch max operations. However, OSS only has one implementation which results in a stable batch max operation.

@A-Stupid-Sun
Copy link
Contributor Author

A-Stupid-Sun commented Jun 5, 2023

Hi, Thanks for the PR! But OSS doesn't need this setting. Maybe you can contact with @manulpatel at PR #2354 to see if you can continue his work?

sorry, i saw the issue #2228 which says s3, oss, a zblob need this setting so i did this, should i close this pr now?

@A-Stupid-Sun
Copy link
Contributor Author

But OSS doesn't need this setting.

Since S3 has different implementations such as AWS S3, R2, etc., we require a configuration value for batch max operations. However, OSS only has one implementation which results in a stable batch max operation.

yes, i read the doc of oss, and don’t find this configuration 😥

@Xuanwo
Copy link
Member

Xuanwo commented Jun 5, 2023

sorry, i saw the issue #2228 which says s3, oss, a zblob need this setting so i did this, should i close this pr now?

Your understanding is correct. I overlooked some context regarding this issue, and now I realize that this PR is necessary. Please accept my apologies for the mistake!

@Xuanwo
Copy link
Member

Xuanwo commented Jun 5, 2023

thanks for your help, i will close it now

Hi, this PR is required. Let's get it merged!

@A-Stupid-Sun
Copy link
Contributor Author

thanks for your help, i will close it now

Hi, this PR is required. Let's get it merged!

my pleasure, i will try my best

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.

Thanks!

@Xuanwo Xuanwo merged commit a3b68fd into apache:main Jun 5, 2023
@A-Stupid-Sun A-Stupid-Sun deleted the fix-oss-batch_max_operations branch June 5, 2023 04:44
@suyanhanx suyanhanx mentioned this pull request Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants