Skip to content

TT-11748 dont attempt to remove ApiCacheDeletion key from redis#6215

Merged
sredxny merged 1 commit into
masterfrom
TT-11748-delete-cache-no-key-removal
Apr 9, 2024
Merged

TT-11748 dont attempt to remove ApiCacheDeletion key from redis#6215
sredxny merged 1 commit into
masterfrom
TT-11748-delete-cache-no-key-removal

Conversation

@sredxny

@sredxny sredxny commented Apr 9, 2024

Copy link
Copy Markdown
Contributor

User description

Description

when the key is to delete cache then we do not need to find the key in redis and remove it as we do for ordinary keys as we will only trigger the process to delete the api cache. Currently we have logs like:

time="Apr 08 19:28:42" level=info msg="--> removing cached key: ****ache"
time="Apr 08 19:28:43" level=error msg="Key not found in master - skipping"

Which are not desired in the given context

Related Issue

TT-11748

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

Type

bug_fix


Description

  • Fixes unnecessary removal attempts for API cache deletion keys by marking them as not regular, thus avoiding error logs for missing keys.

Changes walkthrough

Relevant files
Bug fix
rpc_storage_handler.go
Avoid Unnecessary Removal Attempts for API Cache Deletion Keys

gateway/rpc_storage_handler.go

  • Mark NoticeDeleteAPICache keys as not regular to avoid unnecessary
    removal attempts.
  • +1/-0     

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    …n redis and remove it as we do for ordinary keys
    @github-actions

    github-actions Bot commented Apr 9, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Description updated to latest commit (be06141)

    @github-actions

    github-actions Bot commented Apr 9, 2024

    Copy link
    Copy Markdown
    Contributor

    API Changes

    no api changes detected

    @github-actions

    github-actions Bot commented Apr 9, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    1, because the change is small and straightforward, focusing on a specific bug fix related to cache deletion logic. The logic is clear, and the impact of the change is limited to a specific case, making the review process relatively simple.

    🧪 Relevant tests

    No

    🔍 Possible issues

    No

    🔒 Security concerns

    No

    Code feedback:
    relevant filegateway/rpc_storage_handler.go
    suggestion      

    Consider adding a log statement before appending to apiIDsToDeleteCache for better traceability and debugging. Knowing when an API cache deletion is triggered can be useful for operational monitoring and troubleshooting. [medium]

    relevant linenotRegularKeys[key] = true


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    @github-actions

    github-actions Bot commented Apr 9, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Maintainability
    Add a comment explaining the reasoning behind marking keys as not regular for NoticeDeleteAPICache case.

    Consider adding a comment explaining why notRegularKeys[key] = true is set for
    NoticeDeleteAPICache case. This addition seems to mirror the behavior of other cases, but
    the specific reasoning or implications for this case might not be immediately clear to
    future maintainers or readers of the code.

    gateway/rpc_storage_handler.go [1027]

    +// Explain why we mark this key as not regular in the context of NoticeDeleteAPICache
     notRegularKeys[key] = true
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    @sredxny
    sredxny requested review from jeffy-mathew and padiazg April 9, 2024 02:23
    @sonarqubecloud

    sonarqubecloud Bot commented Apr 9, 2024

    Copy link
    Copy Markdown

    @padiazg padiazg left a comment

    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

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

    LGTM

    @sredxny
    sredxny merged commit 743fe39 into master Apr 9, 2024
    @sredxny
    sredxny deleted the TT-11748-delete-cache-no-key-removal branch April 9, 2024 12:46
    @sredxny

    sredxny commented Apr 9, 2024

    Copy link
    Copy Markdown
    Contributor Author

    /release to release-5.3

    @tykbot

    tykbot Bot commented Apr 9, 2024

    Copy link
    Copy Markdown

    Working on it! Note that it can take a few minutes.

    tykbot Bot pushed a commit that referenced this pull request Apr 9, 2024
    ## **User description**
    <!-- Provide a general summary of your changes in the Title above -->
    
    ## Description
    
    when the key is to delete cache then we do not need to find the key in
    redis and remove it as we do for ordinary keys as we will only trigger
    the process to delete the api cache. Currently we have logs like:
    
    ```
    time="Apr 08 19:28:42" level=info msg="--> removing cached key: ****ache"
    time="Apr 08 19:28:43" level=error msg="Key not found in master - skipping"
    ```
    Which are not desired in the given context
    
    ## Related Issue
    
    TT-11748
    
    ## Motivation and Context
    
    <!-- Why is this change required? What problem does it solve? -->
    
    ## How This Has Been Tested
    
    <!-- Please describe in detail how you tested your changes -->
    <!-- Include details of your testing environment, and the tests -->
    <!-- you ran to see how your change affects other areas of the code,
    etc. -->
    <!-- This information is helpful for reviewers and QA. -->
    
    ## Screenshots (if appropriate)
    
    ## Types of changes
    
    <!-- What types of changes does your code introduce? Put an `x` in all
    the boxes that apply: -->
    
    - [ ] Bug fix (non-breaking change which fixes an issue)
    - [ ] New feature (non-breaking change which adds functionality)
    - [ ] Breaking change (fix or feature that would cause existing
    functionality to change)
    - [ ] Refactoring or add test (improvements in base code or adds test
    coverage to functionality)
    
    ## Checklist
    
    <!-- Go over all the following points, and put an `x` in all the boxes
    that apply -->
    <!-- If there are no documentation updates required, mark the item as
    checked. -->
    <!-- Raise up any additional concerns not covered by the checklist. -->
    
    - [ ] I ensured that the documentation is up to date
    - [ ] I explained why this PR updates go.mod in detail with reasoning
    why it's required
    - [ ] I would like a code coverage CI quality gate exception and have
    explained why
    
    
    ___
    
    ## **Type**
    bug_fix
    
    
    ___
    
    ## **Description**
    - Fixes unnecessary removal attempts for API cache deletion keys by
    marking them as not regular, thus avoiding error logs for missing keys.
    
    
    ___
    
    
    
    ## **Changes walkthrough**
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Bug
    fix</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>rpc_storage_handler.go</strong><dd><code>Avoid
    Unnecessary Removal Attempts for API Cache Deletion
    Keys</code></dd></summary>
    <hr>
    
    gateway/rpc_storage_handler.go
    <li>Mark <code>NoticeDeleteAPICache</code> keys as not regular to avoid
    unnecessary <br>removal attempts.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6215/files#diff-8875f75b602664c44b62b67a4da41d748124ad270573a44db4ec977ee5d68021">+1/-0</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > ✨ **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools
    and their descriptions
    
    (cherry picked from commit 743fe39)
    @tykbot

    tykbot Bot commented Apr 9, 2024

    Copy link
    Copy Markdown

    @sredxny Succesfully merged PR

    @sredxny

    sredxny commented Apr 9, 2024

    Copy link
    Copy Markdown
    Contributor Author

    /release to release-5-lts

    @tykbot

    tykbot Bot commented Apr 9, 2024

    Copy link
    Copy Markdown

    Working on it! Note that it can take a few minutes.

    tykbot Bot pushed a commit that referenced this pull request Apr 9, 2024
    ## **User description**
    <!-- Provide a general summary of your changes in the Title above -->
    
    ## Description
    
    when the key is to delete cache then we do not need to find the key in
    redis and remove it as we do for ordinary keys as we will only trigger
    the process to delete the api cache. Currently we have logs like:
    
    ```
    time="Apr 08 19:28:42" level=info msg="--> removing cached key: ****ache"
    time="Apr 08 19:28:43" level=error msg="Key not found in master - skipping"
    ```
    Which are not desired in the given context
    
    ## Related Issue
    
    TT-11748
    
    ## Motivation and Context
    
    <!-- Why is this change required? What problem does it solve? -->
    
    ## How This Has Been Tested
    
    <!-- Please describe in detail how you tested your changes -->
    <!-- Include details of your testing environment, and the tests -->
    <!-- you ran to see how your change affects other areas of the code,
    etc. -->
    <!-- This information is helpful for reviewers and QA. -->
    
    ## Screenshots (if appropriate)
    
    ## Types of changes
    
    <!-- What types of changes does your code introduce? Put an `x` in all
    the boxes that apply: -->
    
    - [ ] Bug fix (non-breaking change which fixes an issue)
    - [ ] New feature (non-breaking change which adds functionality)
    - [ ] Breaking change (fix or feature that would cause existing
    functionality to change)
    - [ ] Refactoring or add test (improvements in base code or adds test
    coverage to functionality)
    
    ## Checklist
    
    <!-- Go over all the following points, and put an `x` in all the boxes
    that apply -->
    <!-- If there are no documentation updates required, mark the item as
    checked. -->
    <!-- Raise up any additional concerns not covered by the checklist. -->
    
    - [ ] I ensured that the documentation is up to date
    - [ ] I explained why this PR updates go.mod in detail with reasoning
    why it's required
    - [ ] I would like a code coverage CI quality gate exception and have
    explained why
    
    
    ___
    
    ## **Type**
    bug_fix
    
    
    ___
    
    ## **Description**
    - Fixes unnecessary removal attempts for API cache deletion keys by
    marking them as not regular, thus avoiding error logs for missing keys.
    
    
    ___
    
    
    
    ## **Changes walkthrough**
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Bug
    fix</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>rpc_storage_handler.go</strong><dd><code>Avoid
    Unnecessary Removal Attempts for API Cache Deletion
    Keys</code></dd></summary>
    <hr>
    
    gateway/rpc_storage_handler.go
    <li>Mark <code>NoticeDeleteAPICache</code> keys as not regular to avoid
    unnecessary <br>removal attempts.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6215/files#diff-8875f75b602664c44b62b67a4da41d748124ad270573a44db4ec977ee5d68021">+1/-0</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > ✨ **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools
    and their descriptions
    
    (cherry picked from commit 743fe39)
    @tykbot

    tykbot Bot commented Apr 9, 2024

    Copy link
    Copy Markdown

    @sredxny Succesfully merged PR

    buger added a commit that referenced this pull request Apr 9, 2024
    …ion key from redis (#6215)
    
    TT-11748 dont attempt to remove ApiCacheDeletion key from redis (#6215)
    
    ## **User description**
    <!-- Provide a general summary of your changes in the Title above -->
    
    ## Description
    
    when the key is to delete cache then we do not need to find the key in
    redis and remove it as we do for ordinary keys as we will only trigger
    the process to delete the api cache. Currently we have logs like:
    
    ```
    time="Apr 08 19:28:42" level=info msg="--> removing cached key: ****ache"
    time="Apr 08 19:28:43" level=error msg="Key not found in master - skipping"
    ```
    Which are not desired in the given context
    
    ## Related Issue
    
    TT-11748
    
    ## Motivation and Context
    
    <!-- Why is this change required? What problem does it solve? -->
    
    ## How This Has Been Tested
    
    <!-- Please describe in detail how you tested your changes -->
    <!-- Include details of your testing environment, and the tests -->
    <!-- you ran to see how your change affects other areas of the code,
    etc. -->
    <!-- This information is helpful for reviewers and QA. -->
    
    ## Screenshots (if appropriate)
    
    ## Types of changes
    
    <!-- What types of changes does your code introduce? Put an `x` in all
    the boxes that apply: -->
    
    - [ ] Bug fix (non-breaking change which fixes an issue)
    - [ ] New feature (non-breaking change which adds functionality)
    - [ ] Breaking change (fix or feature that would cause existing
    functionality to change)
    - [ ] Refactoring or add test (improvements in base code or adds test
    coverage to functionality)
    
    ## Checklist
    
    <!-- Go over all the following points, and put an `x` in all the boxes
    that apply -->
    <!-- If there are no documentation updates required, mark the item as
    checked. -->
    <!-- Raise up any additional concerns not covered by the checklist. -->
    
    - [ ] I ensured that the documentation is up to date
    - [ ] I explained why this PR updates go.mod in detail with reasoning
    why it's required
    - [ ] I would like a code coverage CI quality gate exception and have
    explained why
    
    
    ___
    
    ## **Type**
    bug_fix
    
    
    ___
    
    ## **Description**
    - Fixes unnecessary removal attempts for API cache deletion keys by
    marking them as not regular, thus avoiding error logs for missing keys.
    
    
    ___
    
    
    
    ## **Changes walkthrough**
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Bug
    fix</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>rpc_storage_handler.go</strong><dd><code>Avoid
    Unnecessary Removal Attempts for API Cache Deletion
    Keys</code></dd></summary>
    <hr>
    
    gateway/rpc_storage_handler.go
    <li>Mark <code>NoticeDeleteAPICache</code> keys as not regular to avoid
    unnecessary <br>removal attempts.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6215/files#diff-8875f75b602664c44b62b67a4da41d748124ad270573a44db4ec977ee5d68021">+1/-0</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > ✨ **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools
    and their descriptions
    buger added a commit that referenced this pull request Apr 9, 2024
    …etion key from redis (#6215)
    
    TT-11748 dont attempt to remove ApiCacheDeletion key from redis (#6215)
    
    ## **User description**
    <!-- Provide a general summary of your changes in the Title above -->
    
    ## Description
    
    when the key is to delete cache then we do not need to find the key in
    redis and remove it as we do for ordinary keys as we will only trigger
    the process to delete the api cache. Currently we have logs like:
    
    ```
    time="Apr 08 19:28:42" level=info msg="--> removing cached key: ****ache"
    time="Apr 08 19:28:43" level=error msg="Key not found in master - skipping"
    ```
    Which are not desired in the given context
    
    ## Related Issue
    
    TT-11748
    
    ## Motivation and Context
    
    <!-- Why is this change required? What problem does it solve? -->
    
    ## How This Has Been Tested
    
    <!-- Please describe in detail how you tested your changes -->
    <!-- Include details of your testing environment, and the tests -->
    <!-- you ran to see how your change affects other areas of the code,
    etc. -->
    <!-- This information is helpful for reviewers and QA. -->
    
    ## Screenshots (if appropriate)
    
    ## Types of changes
    
    <!-- What types of changes does your code introduce? Put an `x` in all
    the boxes that apply: -->
    
    - [ ] Bug fix (non-breaking change which fixes an issue)
    - [ ] New feature (non-breaking change which adds functionality)
    - [ ] Breaking change (fix or feature that would cause existing
    functionality to change)
    - [ ] Refactoring or add test (improvements in base code or adds test
    coverage to functionality)
    
    ## Checklist
    
    <!-- Go over all the following points, and put an `x` in all the boxes
    that apply -->
    <!-- If there are no documentation updates required, mark the item as
    checked. -->
    <!-- Raise up any additional concerns not covered by the checklist. -->
    
    - [ ] I ensured that the documentation is up to date
    - [ ] I explained why this PR updates go.mod in detail with reasoning
    why it's required
    - [ ] I would like a code coverage CI quality gate exception and have
    explained why
    
    
    ___
    
    ## **Type**
    bug_fix
    
    
    ___
    
    ## **Description**
    - Fixes unnecessary removal attempts for API cache deletion keys by
    marking them as not regular, thus avoiding error logs for missing keys.
    
    
    ___
    
    
    
    ## **Changes walkthrough**
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Bug
    fix</strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>rpc_storage_handler.go</strong><dd><code>Avoid
    Unnecessary Removal Attempts for API Cache Deletion
    Keys</code></dd></summary>
    <hr>
    
    gateway/rpc_storage_handler.go
    <li>Mark <code>NoticeDeleteAPICache</code> keys as not regular to avoid
    unnecessary <br>removal attempts.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6215/files#diff-8875f75b602664c44b62b67a4da41d748124ad270573a44db4ec977ee5d68021">+1/-0</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > ✨ **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools
    and their descriptions
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants