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: provide error instead of nil panic when cache_zone is missing #10138

Merged
merged 10 commits into from
Sep 13, 2023

Conversation

Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented Sep 1, 2023

Description

Fixes #9957

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: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
@monkeyDluffy6017 monkeyDluffy6017 added the wait for update wait for the author's response in this issue/PR label Sep 5, 2023
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
@Revolyssup Revolyssup removed the wait for update wait for the author's response in this issue/PR label Sep 5, 2023
kayx23
kayx23 previously approved these changes Sep 5, 2023
t/plugin/proxy-cache/memory.t Outdated Show resolved Hide resolved
enhance test description

Co-authored-by: Abhishek Choudhary <shreemaan.abhishek@gmail.com>
kayx23
kayx23 previously approved these changes Sep 6, 2023
@@ -140,6 +141,12 @@ function _M.check_schema(conf)
end
end

-- For memory based cache, the default cache_zone cannot be used.
-- cache_zone will also be set as default value in case when passed empty.
if conf.cache_strategy == STRATEGY_MEMORY and conf.cache_zone == DEFAULT_CACHE_ZONE then
Copy link
Contributor

@monkeyDluffy6017 monkeyDluffy6017 Sep 12, 2023

Choose a reason for hiding this comment

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

It's not very reliable to judge by cache zone name, i think you could modified like this:

if local_conf.apisix.proxy_cache then
    for _, cache in ipairs(local_conf.apisix.proxy_cache.zones) do
        if cache.name == conf.cache_zone then
            if conf.cache_strategy == STRATEGY_MEMORY and not cache.disk_path then
                found = true
                break
            elseif conf.cache_strategy == STRATEGY_DISK and cache.disk_path then
                found = true
                break
            end
        end
    end
    if found == false then
        return false, "cache_zone " .. conf.cache_zone .. " not found"
    end
end

@monkeyDluffy6017 monkeyDluffy6017 added the wait for update wait for the author's response in this issue/PR label Sep 12, 2023
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
@Revolyssup Revolyssup removed the wait for update wait for the author's response in this issue/PR label Sep 13, 2023
@Revolyssup
Copy link
Contributor Author

@monkeyDluffy6017 I added a slightly modified version of your suggestion with comments. Please review.

@monkeyDluffy6017 monkeyDluffy6017 merged commit 67057b8 into apache:master Sep 13, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants