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 bvt/purge_log (#1.1-dev) #15484

Merged
merged 2 commits into from Apr 12, 2024
Merged

Conversation

YANGGMM
Copy link
Contributor

@YANGGMM YANGGMM commented Apr 12, 2024

changes:

  1. get the newest collecttime as target time to check result.

Approved by: @heni02

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #15455

What this PR does / why we need it:

fix bvt/purge_log

changes:
1. get the newest `collecttime` as target time to check result.

Approved by: @heni02
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Apr 12, 2024
@mergify mergify bot requested a review from sukki37 April 12, 2024 03:43
@mergify mergify bot added the kind/bug Something isn't working label Apr 12, 2024
@matrix-meow
Copy link
Contributor

@YANGGMM Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that it is fixing an issue related to bvt/purge_log.

Body:

The body of the pull request provides a brief overview of the changes made, mentions the issue number that is being addressed (issue #15455), and specifies the type of PR (BUG). It also includes the approval information. The body is informative and to the point.

Changes:

  1. In the func_purge_log.result file:
    • Line 29: Changed the interval from 1 minute to 5 minutes in the SQL query.
  2. In the func_purge_log.sql file:
    • Line 25: Changed the interval from 1 minute to 5 minutes in the SQL query.
  3. In the system_table_cases.sql file:
    • Updated comments and queries related to span_info table.

Feedback and Suggestions for Improvement:

  1. SQL Injection Risk:

    • The use of IFNULL in the SQL query to handle potential NULL values is good practice. However, it's important to ensure that the input values are properly sanitized to prevent SQL injection attacks. Consider using parameterized queries or input validation to mitigate this risk.
  2. Magic Numbers:

    • The interval values (1 day, 5 minutes) are hardcoded in multiple places. It would be beneficial to define these values as constants or variables at the beginning of the file to improve code readability and maintainability.
  3. Comment Clarity:

    • The comments in the system_table_cases.sql file mention issues like "11,947" without clear context. It's recommended to provide more descriptive comments to explain the purpose of the queries and their relation to specific issues for better understanding.
  4. Consistency in Comments:

    • Ensure consistency in commenting style throughout the codebase. For example, use a consistent format for mentioning issues (e.g., -- issue 11,947 vs. -- issue 11,947 end).
  5. Optimization:

    • Consider consolidating repetitive queries or code blocks to reduce redundancy and improve code maintainability. For instance, the similar COUNT queries in system_table_cases.sql could be combined into a single query with different conditions.
  6. Testing:

    • Ensure that appropriate testing is conducted to validate the changes made in the PR, especially considering the impact on the purge_log functionality.
  7. Documentation:

    • Update any relevant documentation or README files to reflect the changes made in this PR, especially if it affects the usage or behavior of the purge_log functionality.

By addressing these points, the code quality, security, and maintainability of the bvt/purge_log functionality can be enhanced.

@mergify mergify bot merged commit d02a0bf into matrixorigin:1.1-dev Apr 12, 2024
18 checks passed
@YANGGMM YANGGMM deleted the fix-date-add-1.1 branch May 13, 2024 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants