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 retry_timeout for upstream #2106

Closed
wants to merge 3 commits into from
Closed

feat: add retry_timeout for upstream #2106

wants to merge 3 commits into from

Conversation

zaunist
Copy link
Contributor

@zaunist zaunist commented Aug 30, 2021

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

Why submit this pull request?

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

What changes will this PR take into?

Add the retry_timeout field to upstream.

Related issues

fix/resolve #2089

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 Aug 30, 2021

✔️ Deploy Preview for apisix-dashboard ready!

🔨 Explore the source changes: 0e9dfa7

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

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

@liuxiran
Copy link
Contributor

Hi @zaunist , please add the related test case for your feature, you can try to test create/configure upstream with retry_timeout, then create/configure upstream successfully, that will help us merge your feature smoothly, thanks a lot

@zaunist
Copy link
Contributor Author

zaunist commented Aug 31, 2021

Hi @zaunist , please add the related test case for your feature, you can try to test create/configure upstream with retry_timeout, then create/configure upstream successfully, that will help us merge your feature smoothly, thanks a lot

Okay, give me some time to add after I learn to write test cases, tomorrow or later

@liuxiran
Copy link
Contributor

Hi @zaunist , please add the related test case for your feature, you can try to test create/configure upstream with retry_timeout, then create/configure upstream successfully, that will help us merge your feature smoothly, thanks a lot

Okay, give me some time to add after I learn to write test cases, tomorrow or later

got you, and you can refer line 60 in https://github.com/apache/apisix-dashboard/blob/master/web/cypress/integration/upstream/create_and_delete_upstream.spec.js, it is a good example, any problem can comment here

@zaunist
Copy link
Contributor Author

zaunist commented Sep 1, 2021

When I was writing the test cases, I found some other problems, such as schema.json was not fully updated, so I updated schema.json again

weight0: '2',
port1: '7001',
weight1: '2',
retry_timeout: '3',
Copy link
Contributor

Choose a reason for hiding this comment

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

since retry_timeout has been defined, we can use it in the Line60, the same goes for other test data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is only a variable defined in data that can be used, but I don't use it, is that right?

Copy link
Contributor

Choose a reason for hiding this comment

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

if you define a variable, it would be better to use it, or this variable is useless here

@liuxiran
Copy link
Contributor

liuxiran commented Sep 1, 2021

When I was writing the test cases, I found some other problems, such as schema.json was not fully updated, so I updated schema.json again

How careful you are~! and since our version 2.8 will be published soon, we will have a pr to update schema.json consistently, after that you can try to sync the code, thanks very much

@zaunist
Copy link
Contributor Author

zaunist commented Sep 1, 2021

When I was writing the test cases, I found some other problems, such as schema.json was not fully updated, so I updated schema.json again

How careful you are~! and since our version 2.8 will be published soon, we will have a pr to update schema.json consistently, after that you can try to sync the code, thanks very much

ok :)

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2021

Codecov Report

Merging #2106 (5389b93) into master (709d815) will decrease coverage by 1.69%.
The diff coverage is n/a.

❗ Current head 5389b93 differs from pull request most recent head 0e9dfa7. Consider uploading reports for the commit 0e9dfa7 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2106      +/-   ##
==========================================
- Coverage   66.63%   64.93%   -1.70%     
==========================================
  Files         121       60      -61     
  Lines        3231     3779     +548     
  Branches      787        0     -787     
==========================================
+ Hits         2153     2454     +301     
+ Misses       1078     1043      -35     
- Partials        0      282     +282     
Flag Coverage Δ
backend-e2e-test 47.10% <ø> (?)
backend-unit-test 52.98% <ø> (?)
frontend-e2e-test ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/internal/core/entity/entity.go 77.27% <ø> (ø)
web/src/components/Plugin/UI/proxy-mirror.tsx
...pages/Route/components/CreateStep4/CreateStep4.tsx
web/src/components/RightContent/index.tsx
web/src/components/HeaderDropdown/index.tsx
...mponents/Upstream/components/active-check/Host.tsx
...eb/src/pages/PluginTemplate/components/Preview.tsx
web/src/components/RawDataEditor/RawDataEditor.tsx
web/src/pages/Dashboard/service.ts
...Route/components/DebugViews/AuthenticationView.tsx
... and 163 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcdd3b6...0e9dfa7. Read the comment docs.

@zaunist zaunist closed this Sep 5, 2021
@liuxiran
Copy link
Contributor

liuxiran commented Sep 7, 2021

Hi @zaunist , sorry to cover your code by #2117

at that time we are dealing with the last pr of release 2.8, which was blocked by the CI problem, in order to fix that we rewrite your code without communicating with you since it was too late that night.

and thanks very much for your interest and contribution in the project, and there are still many issues to be solved, welcome to take any of them you are interested in.

@zaunist
Copy link
Contributor Author

zaunist commented Sep 7, 2021

Hi @zaunist , sorry to cover your code by #2117

at that time we are dealing with the last pr of release 2.8, which was blocked by the CI problem, in order to fix that we rewrite your code without communicating with you since it was too late that night.

and thanks very much for your interest and contribution in the project, and there are still many issues to be solved, welcome to take any of them you are interested in.

Thanks for your comment, maybe next time I can contribute code to the project

@liuxiran
Copy link
Contributor

liuxiran commented Sep 8, 2021

Hi @zaunist I found that most of the projects under your name are related to go, so you will be more interested in go related issues right? If so I will also invite you to help related problems, thanks

@zaunist
Copy link
Contributor Author

zaunist commented Sep 8, 2021

Hi @zaunist I found that most of the projects under your name are related to go, so you will be more interested in go related issues right? If so I will also invite you to help related problems, thanks
Okay, let me know if you need anything. I'll do what I can.

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.

Feature request: add retry_timeout for upstream
4 participants