You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/04/26 07:35:06 UTC
[GitHub] [apisix] zhendongcmss opened a new pull request, #6934: fix(balancer): Use default priority when setting upstream doesn't specify priority
zhendongcmss opened a new pull request, #6934:
URL: https://github.com/apache/apisix/pull/6934
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
The `transform_node` function doesn't check the priority. If the priority is `nil`, apisix will print a error.
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on pull request #6934: fix(balancer): Add test cases when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1125577027
@tzssangglass @spacewander please review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #6934: fix(balancer): Use default priority when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1109885097
hi @zhendongcmss we need test cases to cover this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #6934: fix(balancer): Use default priority when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1110845400
> I know that.
> But, Somtimes, we develop a new plugin on access phase and use `upstream.set(ctx, "xxxx", "xxx", up_conf)` to rewrite the upstreams, if the `up_conf` doesn't include the priority(want to use the default priority) will get an error.
I don't really agree with the way this was handled. I think it's too late to check the parameters here.
In fact, for all configurations with default attributes that are not verified by the json schema, there are similar problems?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #6934: fix(balancer): Add test cases when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1125584670
Please make the CI pass, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on pull request #6934: fix(balancer): Use default priority when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1110603735
> I'm curious about the trigger condition because priority is set as a default in the JSON schema. see:
>
> https://github.com/apache/apisix/blob/e56d23950ff18f7656a605ad80b5c9a2576d3c01/apisix/schema_def.lua#L331-L335
I know that.
But, Somtimes, we develop a new plugin on access phase and use `upstream.set(ctx, "xxxx", "xxx", up_conf)` to rewrite the upstreams, if the `up_conf` doesn't include the priority(want to use the default priority) will get a error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on a diff in pull request #6934: fix(balancer): Add test cases when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on code in PR #6934:
URL: https://github.com/apache/apisix/pull/6934#discussion_r863520908
##########
apisix/balancer.lua:
##########
@@ -50,12 +50,17 @@ local function transform_node(new_nodes, node)
new_nodes._priority_index = {}
end
- if not new_nodes[node.priority] then
- new_nodes[node.priority] = {}
- core.table.insert(new_nodes._priority_index, node.priority)
+ local node_priority = 0 -- if nodes don't specify priority using default
+ if node.priority then
Review Comment:
If you don't agree with that add the priority. I think it is necessary to add test cases to test it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #6934: fix(balancer): Use default priority when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1110465905
I'm curious about the trigger condition because priority is set as a default in the JSON schema.
see: https://github.com/apache/apisix/blob/e56d23950ff18f7656a605ad80b5c9a2576d3c01/apisix/schema_def.lua#L331-L335
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #6934: fix(balancer): Use default priority when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #6934:
URL: https://github.com/apache/apisix/pull/6934#discussion_r860525004
##########
apisix/balancer.lua:
##########
@@ -50,12 +50,17 @@ local function transform_node(new_nodes, node)
new_nodes._priority_index = {}
end
- if not new_nodes[node.priority] then
- new_nodes[node.priority] = {}
- core.table.insert(new_nodes._priority_index, node.priority)
+ local node_priority = 0 -- if nodes don't specify priority using default
+ if node.priority then
Review Comment:
Is it possible to call https://github.com/apache/apisix/blob/353e10b60e3604f80bae173a07c3c570f38ae367/apisix/upstream.lua#L179?
We already have a fixing logic in https://github.com/apache/apisix/blob/353e10b60e3604f80bae173a07c3c570f38ae367/apisix/upstream.lua#L219.
It will be terrible to fill a default value in three places.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on pull request #6934: fix(balancer): Use default priority when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1111018811
> schema
I think it is necessary to check the passed parameter to aviod thread crash error.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on pull request #6934: fix(balancer): Use default priority when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1111201853
Let's hear what others think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #6934: fix(balancer): Add test cases when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1217821439
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on pull request #6934: fix(balancer): Use default priority when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1110427760
> hi @zhendongcmss we need test cases to cover this.
will update
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] zhendongcmss commented on pull request #6934: fix(balancer): Add test cases when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1127366563
> Please make the CI pass, thanks!
There no default priority checking the test cases don't pass. How about you that the test cases are necessary and reasonable ？
The test cases call test->balancer.pick_sever -> create_server_picker -> fetch_health_nodes, there are no default priority check.
I think it is reasonable because some times we call the API function not only using jsom conf.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #6934: fix(balancer): Add test cases when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #6934:
URL: https://github.com/apache/apisix/pull/6934#issuecomment-1187019634
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] closed pull request #6934: fix(balancer): Add test cases when setting upstream doesn't specify priority
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #6934: fix(balancer): Add test cases when setting upstream doesn't specify priority
URL: https://github.com/apache/apisix/pull/6934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org