Revert "sync to 1.2: remove the old fast ranges and update metrics (#…

[gouhongshen]

…16359)"

This reverts commit 918a356.

What type of PR is this?

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

Which issue(s) this PR fixes:

issue ##16379

What this PR does / why we need it:

revert to avoid oom

[matrix-meow]

@gouhongshen Thanks for your contributions!

Here are review comments for file pkg/util/metric/v2/dashboard/grafana_dashboard_txn.go:

Pull Request Review:

1. Title and Body:

   • The title and body of the pull request indicate that it is a revert of a previous commit to avoid out-of-memory issues.
   • The provided information is clear and concise, stating the purpose of the revert and referencing the related issue.
   • No specific details are mentioned about the changes being reverted, which could be helpful for context.

2. Changes in grafana_dashboard_txn.go:

   • Problem 1: Code Duplication:

     • The functions initTxnTableRangesRow and initRangesRow seem to have overlapping functionality related to ranges and selectivity metrics.
     • This duplication can lead to maintenance issues and confusion in the codebase.

   • Suggestion 1:

     • Consider refactoring the code to consolidate the common functionality into a single function to avoid redundancy and improve code maintainability.

   • Problem 2: Inconsistent Naming:

     • The function initFastRangesRow seems to be related to ranges but is named differently from other similar functions.
     • Inconsistent naming conventions can make the codebase harder to understand and maintain.

   • Suggestion 2:

     • Ensure consistent naming conventions across functions to enhance code readability and maintainability.

   • Problem 3: Security Concern - Metric Filtering:

     • The use of getMetricWithFilter with raw filter strings like type="block_selectivity" can pose a security risk if the filter strings are user-controlled.
     • Directly injecting user input into metric queries can lead to injection vulnerabilities.

   • Suggestion 3:

     • Implement proper input validation and sanitization to prevent injection attacks. Consider using parameterized queries or predefined filter options to mitigate this risk.

3. Optimization Suggestions:

   • Optimization 1:
     • Instead of having multiple separate functions for similar metric rows, consolidate related metrics into reusable functions to reduce code duplication and improve maintainability.
   • Optimization 2:
     • Consider abstracting common functionality into helper functions to promote code reusability and enhance readability.

Overall Comments:

• The revert seems to address an urgent issue related to memory consumption, which is crucial for system stability.
• The code changes in grafana_dashboard_txn.go exhibit some issues related to code duplication, inconsistent naming, and potential security vulnerabilities.
• It is recommended to address the identified problems and consider optimizing the code for better maintainability and security.
• Providing more detailed comments in the code regarding the purpose of each metric row can also aid in understanding and future modifications.

By addressing the mentioned problems and implementing the suggested optimizations, the codebase can be enhanced in terms of clarity, security, and maintainability.

Here are review comments for file pkg/util/metric/v2/metrics.go:

Pull Request Review:

Title:

The title indicates that the pull request is reverting a previous commit related to removing old fast ranges and updating metrics.

Body:

The body of the pull request explains that the revert is necessary to avoid out-of-memory (OOM) issues. It references the specific issue #16379 on GitHub that this PR is addressing.

Changes in pkg/util/metric/v2/metrics.go:

1. Problem:

   • The reverted commit removed txnTableRangeTotalHistogram and added txnTableRangeSizeHistogram.
   • Additionally, it added TxnRangesLoadedObjectMetaTotalCounter.

2. Suggestions:

   • It's important to understand why the revert is necessary to avoid OOM issues. Ensure that the memory usage implications of the changes are thoroughly understood.
   • Reverting changes should be a temporary solution. Consider revisiting the changes that were reverted and find a way to address the OOM issues without losing the improvements made in the previous commit.
   • Review the changes made in the reverted commit to identify any potential optimizations or alternative approaches that could be taken to mitigate memory usage concerns.
   • Ensure that the metrics being registered are accurate and necessary for monitoring and debugging purposes.

3. Security Concerns:

   • Ensure that the changes do not introduce any security vulnerabilities or data leakage risks. Review the impact of metric changes on sensitive data handling.

4. Optimization:

   • Consider optimizing the metrics registration process if there are redundancies or unnecessary metrics being tracked. This can help improve performance and resource utilization.

General Suggestions:

• Provide more detailed information in the pull request description about the specific memory issues that led to the revert decision.
• Encourage communication between team members to discuss alternative solutions to address memory concerns without losing valuable changes.

By addressing the above points, the pull request can be improved in terms of code quality, performance, and overall effectiveness in resolving the memory issues.

Here are review comments for file pkg/util/metric/v2/txn.go:

Pull Request Review:

Title:

The title indicates that the pull request is reverting a specific commit related to removing old fast ranges and updating metrics.

Body:

The body of the pull request provides a brief explanation that the revert is necessary to avoid running out of memory (oom).

Changes in pkg/util/metric/v2/txn.go:

1. Addition of TxnRangesLoadedObjectMetaTotalCounter:

   • Issue: The addition of this counter may not be necessary for the current revert action.
   • Suggestion: Consider removing this addition as it does not seem related to the revert purpose.

2. Changes in txnTableRangeSizeHistogram:

   • Issue: The histogram name and help text have been changed, but the labels and buckets remain the same.
   • Suggestion: Ensure that the changes in the histogram name and help text align with the actual purpose of the histogram.

3. Changes in TxnRangeSizeHistogram and TxnFastRangeSizeHistogram:

   • Issue: The labels for these histograms have been changed, but the purpose of these changes is not clear.
   • Suggestion: Verify if the label changes are necessary for the revert action or if they are related to other ongoing developments.

4. Changes in txnTNSideDurationHistogram:

   • Issue: The buckets for this histogram have been changed from exponential to linear, which may impact the distribution of data.
   • Suggestion: Ensure that the bucket changes align with the intended data distribution and performance requirements.

5. Changes in TxnRangesBlockSelectivityHistogram, TxnFastRangesBlockSelectivityHistogram, and TxnFastRangesZMapSelectivityHistogram:

   • Issue: The labels for these histograms have been modified, but the reasoning behind these changes is not clear.
   • Suggestion: Confirm if the label modifications are necessary for the revert or if they are part of a separate enhancement.

Overall Suggestions for Optimization:

• Review the changes made in the file txn.go to ensure they align with the purpose of the revert.
• Remove any additions or modifications that are not directly related to the revert action to maintain code clarity and simplicity.
• Provide detailed comments or documentation explaining the rationale behind each change to facilitate future reviews and maintenance.

By addressing the identified issues and optimizing the changes, the codebase can be maintained efficiently and effectively.

Here are review comments for file pkg/vm/engine/disttae/filter.go:

Pull Request Review:

Title and Body:

The title of the pull request indicates that it is reverting a previous commit with the intention of avoiding an out-of-memory (OOM) issue. The body provides additional context by mentioning the specific issue it fixes and the reason for the revert. This information is clear and helpful in understanding the purpose of the pull request.

Changes in pkg/vm/engine/disttae/filter.go:

1. Removed Metrics Related Code:

   • The changes involve removing several lines of code related to metrics tracking in the ExecuteBlockFilter function.
   • Variables like totalBlocks, loadHit, objFilterTotal, objFilterHit, blkFilterTotal, and blkFilterHit have been removed.
   • Deferred functions that were used to observe metrics have been eliminated.
   • Increment and observation logic for various metrics have been removed throughout the function.

2. Fast Filter, Object Filter, and Block Filter Logic:

   • The logic related to fast filtering, object filtering, and block filtering has been modified.
   • Incrementing and observing metrics based on the success of these filters have been removed.
   • Conditions that previously triggered metric observations have been simplified or removed.

Suggestions for Improvement:

1. Optimizing Metric Handling:

   • Instead of completely removing the metric tracking code, consider refactoring it to be more efficient or to handle metrics in a better way.
   • If metrics are essential for monitoring system performance, find a way to retain them without causing performance overhead.

2. Code Cleanup and Refactoring:

   • The removal of unnecessary variables and logic is a good step, but ensure that the code remains clean and readable after the changes.
   • Consider refactoring the function to improve readability and maintainability, especially after removing the metric-related code.

3. Security Consideration:

   • While the changes in this pull request do not introduce security vulnerabilities directly, it is crucial to ensure that metric handling or any other modifications do not inadvertently impact system security.

4. Documentation Update:

   • If metrics are being handled differently or removed, update the relevant documentation to reflect these changes for future reference.

Overall:

The pull request effectively reverts the commit to address an OOM issue, but the removal of metric tracking code may impact system monitoring and performance analysis. It is essential to carefully consider the implications of removing metrics and to optimize the code changes for better efficiency and maintainability.

By addressing the suggestions provided, the codebase can be improved in terms of performance, readability, and maintainability while ensuring that system monitoring capabilities are not compromised.

Here are review comments for file pkg/vm/engine/disttae/reader.go:

Pull Request Review:

Title and Body:

The title and body of the pull request indicate that the commit is being reverted to avoid out-of-memory (OOM) issues. This is a common practice to address critical problems that have been introduced by a previous change. The body provides context on the reason for the revert and references the related issue. It's good to see clear communication regarding the purpose of the revert.

Changes in pkg/vm/engine/disttae/reader.go:

1. Code Revert and Modification:

   • The changes in the file reader.go involve reverting some code back to its previous state and making modifications to handle composite primary keys.
   • The getReadFilter function now checks for pk == nil instead of mixin.filterState.expr == nil || pk == nil, which simplifies the condition.
   • Two new functions getCompositPKFilter and getNonCompositPKFilter have been added to handle composite primary keys and non-composite primary keys, respectively.

2. Security Concerns:

   • The addition of code related to composite primary keys does not introduce any obvious security vulnerabilities based on the provided diff.

3. Code Optimization:

   • The code could be optimized by potentially combining the logic for composite and non-composite primary keys into a single function if possible, to reduce code duplication and improve maintainability.

4. Consistency and Readability:

   • Ensure consistent naming conventions and formatting throughout the file for better readability and maintainability.
   • Consider adding comments to explain complex logic or algorithms to aid understanding for future developers.

Suggestions for Improvement:

1. Optimization:

   • Consider refactoring the code to reduce redundancy and improve code maintainability. If possible, consolidate common logic for composite and non-composite primary keys to avoid duplication.
   • Ensure that the code follows consistent naming conventions and formatting standards for better readability.

2. Documentation:

   • Add or update comments to explain the purpose and functionality of newly added or modified functions, especially getCompositPKFilter and getNonCompositPKFilter, to aid in understanding the code.

3. Testing:

   • Ensure that appropriate tests are added or updated to cover the changes made in this pull request, especially for handling composite primary keys.

4. Review and Collaboration:

   • Encourage collaboration and code review among team members to gather feedback and ensure the changes align with the project's coding standards and requirements.

By addressing the suggestions mentioned above, the codebase can be improved in terms of readability, maintainability, and efficiency. It's essential to prioritize code quality and security while making changes to the codebase.