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

feat:add upload download add bandwidth limit #411

Merged
merged 4 commits into from
May 12, 2023
Merged

Conversation

constwz
Copy link
Contributor

@constwz constwz commented May 10, 2023

Description

Upload and download with broadband stream implementation

Rationale

Protect the sp service

Example

Limit the upload and download speed

Changes

Notable changes:

  • Upload and download with broadband stream implementation

@constwz constwz force-pushed the feat-bandwidth-limit branch from 2aeb8d7 to ed9edae Compare May 10, 2023 09:47
config/config.go Outdated
@@ -44,6 +44,7 @@ type StorageProviderConfig struct {
RateLimiter *localhttp.RateLimiterConfig
DiscontinueCfg *stopserving.DiscontinueConfig
MetadataCfg *metadata.MetadataConfig
BandwidthLimiter *localhttp.BandWidthLimiterConfig
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rename BandWidthLimiterConfig to BandwidthLimiterConfig .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -208,6 +210,10 @@ var DefaultRateLimiterConfig = &localhttp.RateLimiterConfig{
},
}

var DefaultBandwidthLimiterConfig = &localhttp.BandWidthLimiterConfig{
Enable: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you need to update the default configuration synchronously at config/config_template.toml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

type BandWidthLimiterConfig struct {
Enable bool
R rate.Limit
B int
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add more commentaries to explain the config meaning.
This naming convention is a bit vague for the name that the user needs to configure, and the user may not understand 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.

Annotated

// If it is limited, it will block
if localHttp.BandWidthLimit != nil {
if err := localHttp.BandWidthLimit.Limiter.Wait(ctx); err != nil {
log.Errorw("failed to Wait bandwidth limiter", "err", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Refine Wait to wait, err to error in log.

@@ -223,6 +229,12 @@ func (gateway *Gateway) putObjectHandler(w http.ResponseWriter, r *http.Request)
return
}
if readN > 0 {
// If it is limited, it will block
if localHttp.BandWidthLimit != nil {
if err := localHttp.BandWidthLimit.Limiter.Wait(ctx); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand how to calculate the actual bandwidth of the service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first version of the implementation was simple and limited to whether data could be read from the network

@constwz constwz force-pushed the feat-bandwidth-limit branch from ed9edae to 56ac4f7 Compare May 10, 2023 10:32
@constwz constwz added the r4r Ready for review label May 10, 2023
@krish-nr krish-nr self-requested a review May 11, 2023 03:46
Copy link
Contributor

@krish-nr krish-nr left a comment

Choose a reason for hiding this comment

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

LGTM

@constwz constwz requested a review from joeylichang May 11, 2023 03:47
@constwz constwz force-pushed the feat-bandwidth-limit branch from abea273 to abe9cc1 Compare May 11, 2023 09:02
@constwz constwz force-pushed the feat-bandwidth-limit branch from abe9cc1 to 476c356 Compare May 11, 2023 09:13
@will-2012 will-2012 requested review from fynnss and ruojunm May 12, 2023 01:18
@constwz constwz merged commit b82a528 into develop May 12, 2023
@sysvm sysvm deleted the feat-bandwidth-limit branch May 26, 2023 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
r4r Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants