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: set a initial weight value for the upstream node #1979

Merged
merged 2 commits into from
Jul 21, 2021
Merged

feat: set a initial weight value for the upstream node #1979

merged 2 commits into from
Jul 21, 2021

Conversation

okaybase
Copy link
Member

@okaybase okaybase commented Jul 13, 2021

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?
For most scenarios, the weight value of the upstream node is 1, so setting a initial weight value 1 can reduce user input.

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What changes will this PR take into?

Please update this section with detailed description.

Related issues

fix/resolve
image

Checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@netlify
Copy link

netlify bot commented Jul 13, 2021

✔️ Deploy Preview for apisix-dashboard ready!

🔨 Explore the source changes: adba408

🔍 Inspect the deploy log: https://app.netlify.com/sites/apisix-dashboard/deploys/60edc4732a3ffc000743a328

😎 Browse the preview: https://deploy-preview-1979--apisix-dashboard.netlify.app

@iamayushdas
Copy link
Contributor

iamayushdas commented Jul 13, 2021

@okaybase I don't think its the need right now, also doing break change to code directly will ruin the tests also

cc @juzhiyuan @Yiyiyimu

@okaybase
Copy link
Member Author

Thanks for your review firstly. @iamayushdas
Just like the initial value for connecting/sending/reading timeout, by setting a initial weight value for the upstream node, can reduce user input.

@juzhiyuan
Copy link
Member

Thanks for your review firstly. @iamayushdas
Just like the initial value for connecting/sending/reading timeout, by setting a initial weight value for the upstream node, can reduce user input.

I'm not sure if this will take a break change for users 🤔 need @liuxiran and @nic-chen to confirm.

@nic-chen
Copy link
Member

Thanks for your review firstly. @iamayushdas

Just like the initial value for connecting/sending/reading timeout, by setting a initial weight value for the upstream node, can reduce user input.

I'm not sure if this will take a break change for users 🤔 need @liuxiran and @nic-chen to confirm.

If it fill in the default value automatically when weight is empty, I think it's ok.

@iamayushdas
Copy link
Contributor

Okay then

@juzhiyuan
Copy link
Member

so should we allow the default value 1? @nic-chen

@nic-chen
Copy link
Member

so should we allow the default value 1? @nic-chen

1 is OK.

It’s just that if the weight is filled in by default, users may forget to change it in by themselves. I also hesitate about this.

@liuxiran
Copy link
Contributor

Thanks for your review firstly. @iamayushdas
Just like the initial value for connecting/sending/reading timeout, by setting a initial weight value for the upstream node, can reduce user input.

I'm not sure if this will take a break change for users 🤔 need @liuxiran and @nic-chen to confirm.

I want to make sure that when there is only one node in the upstream, the value of the weight is of little significance right @nic-chen , and when there are several nodes in the upstream, the user would be an advanced user IMO, he/she would know how to set the wight.

So set default value for weight is good for me.

@okaybase
Copy link
Member Author

Thanks for your review firstly. @juzhiyuan @nic-chen @liuxiran

In a production environment, the number of upstream nodes is usually large in order to solve single-point problems and handle large request traffic.

@okaybase
Copy link
Member Author

Thanks for your approval. @juzhiyuan @iamayushdas

@juzhiyuan juzhiyuan merged commit c01fd52 into apache:master Jul 21, 2021
@okaybase okaybase deleted the okaybase20210713 branch July 21, 2021 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants