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(nacos): add skipped nil check for "applications" upvalue #9960

Merged
merged 4 commits into from
Aug 8, 2023

Conversation

Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented Aug 3, 2023

Description

Fixes #9527 (issue)
After the 5 second wait time, it can be possible that applications upvalue has still not being populated and is nil. So it should be checked.

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 first)

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@monkeyDluffy6017 monkeyDluffy6017 changed the title fix: add skipped nil check for "applications" upvalue fix(nacos): add skipped nil check for "applications" upvalue Aug 3, 2023
AlinsRan
AlinsRan previously approved these changes Aug 3, 2023
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
return nil
if not applications or not applications[namespace_id]
or not applications[namespace_id][group_name] then
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

four spaces are needed

Copy link
Contributor

Choose a reason for hiding this comment

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

if not applications or not applications[namespace_id]
    or not applications[namespace_id][group_name] 
then
    return nil

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@monkeyDluffy6017 monkeyDluffy6017 added wait for update wait for the author's response in this issue/PR and removed approved labels Aug 3, 2023
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
@monkeyDluffy6017 monkeyDluffy6017 added approved and removed wait for update wait for the author's response in this issue/PR labels Aug 3, 2023
@monkeyDluffy6017
Copy link
Contributor

@Revolyssup please merge the master branch

@Revolyssup
Copy link
Contributor Author

@Revolyssup please merge the master branch

done

@monkeyDluffy6017 monkeyDluffy6017 merged commit f25b09e into apache:master Aug 8, 2023
33 checks passed
rubikplanet pushed a commit to rubikplanet/apisix that referenced this pull request Aug 8, 2023
@lakewatcher
Copy link

lakewatcher commented Aug 8, 2023

我有一点疑问:仅仅增加非空判断是否依然不能解决问题。因为在高并发情况下,我从日志上看到:
1.多进程同时在发起请求和抛出错误。
2.单个进程因获取了nil而不断请求和抛出错误。那么是否会把并发的压力也传到到了服务发现上面,并且让一时的波动导致的错误让此进程一直返回错误。这是否会造成性能瓶颈。

是否应该异步刷新,增加异步刷新结果、刷新时间和空值次数的控制。
即: 1.单位时间段控制异步刷新频率【我看到配置里面的lua_shared_dict.discovery共享内存,那么请求过多也没用】。2.增加结果的时间戳每次请求只用判断时间戳发起异步刷新就行,不用影响本次请求。3.多次返回空值那就真的没发现,抛出错误是正确的逻辑。

我对lua不太了解,可能会有常识错误。如有问题请不要见怪

I have a question about whether simply adding nonempty judgments still won't solve the problem. Because in the case of high concurrency, I see from the log:

  1. Multiple processes are simultaneously initiating requests and throwing errors.
  2. A single process keeps asking for and throwing errors because it gets nil. Does that put concurrent pressure on service discovery and let the process keep returning errors due to temporary fluctuations? Whether this creates a performance bottleneck.

Whether to refresh asynchronously, increase the control of asynchronous refresh result, refresh time, and null times.
That is: 1. Control the asynchronous refresh frequency per unit period [I see the lua_shared_dict.discovery shared memory in the configuration, so too many requests are of no use]. 2. Add the timestamp of the result. You only need to determine the timestamp for each request and initiate asynchronous refreshing. 3. Return a null value many times then you really did not find that throwing an error is the correct logic.

I don't know much about lua and there may be common sense errors. Please don't be offended if you have any questions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants