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-cpu-stats #14105

Merged
merged 3 commits into from
Jan 10, 2024
Merged

fix-cpu-stats #14105

merged 3 commits into from
Jan 10, 2024

Conversation

Morranto
Copy link
Contributor

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #13211 #13982

What this PR does / why we need it:

improve cpu stats by subtracting io duration

(cherry picked from commit cd01e8b)
(cherry picked from commit 49976e1)
@mergify mergify bot requested a review from sukki37 January 10, 2024 09:35
@mergify mergify bot added kind/bug Something isn't working kind/enhancement labels Jan 10, 2024
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jan 10, 2024
@matrix-meow
Copy link
Contributor

@Morranto Thanks for your contributions!

Title: fix-cpu-stats

Body: This pull request fixes a bug related to CPU stats by subtracting IO duration. It also includes improvements to the codebase.

Changes:

  1. In local_fs.go, a new import statement is added for github.com/matrixorigin/matrixone/pkg/util/trace/impl/motrace/statistic.
  2. In the Read function of local_fs.go, a new line of code is added to subtract the IO duration from the CPU stats.
  3. In s3_fs.go, a new import statement is added for github.com/matrixorigin/matrixone/pkg/util/trace/impl/motrace/statistic.
  4. In the read function of s3_fs.go, a new line of code is added to subtract the IO duration from the CPU stats.
  5. In mysql_cmd_executor.go, the calculation of CPU stats is modified to subtract the disk access time and S3 access time from the total time consumed.
  6. In stats_array.go, two new methods are added to the StatsInfo struct to add the disk access time and S3 access time to the CPU stats.

Overall, these changes aim to improve the accuracy of CPU stats by subtracting the IO duration and disk access time from the total time consumed.

Review:

  1. In local_fs.go and s3_fs.go, the import statement for github.com/matrixorigin/matrixone/pkg/util/trace/impl/motrace/statistic is added. This import seems to be necessary for the new methods added to the StatsInfo struct in stats_array.go. However, it would be helpful to add a comment explaining the purpose of this import.

  2. In the Read function of local_fs.go and the read function of s3_fs.go, the IO duration is subtracted from the CPU stats. This change seems reasonable and should improve the accuracy of the CPU stats. However, it would be beneficial to add a comment explaining the reason for this subtraction and how it affects the CPU stats.

  3. In mysql_cmd_executor.go, the calculation of CPU stats is modified to subtract the disk access time and S3 access time from the total time consumed. This change seems logical and should provide a more accurate representation of the CPU stats. However, it would be helpful to add a comment explaining the reason for this modification and how it affects the CPU stats.

  4. In stats_array.go, two new methods (AddS3AccessTimeConsumption and AddDiskAccessTimeConsumption) are added to the StatsInfo struct. These methods allow adding the disk access time and S3 access time to the CPU stats. This change seems reasonable and provides a convenient way to update the CPU stats. However, it would be beneficial to add a comment explaining the purpose of these methods and how they should be used.

Suggestions for optimization:

  1. Consider adding comments to explain the purpose and usage of the new import statement in local_fs.go and s3_fs.go, as well as the new methods in stats_array.go. This will help other developers understand the code and its intentions.

  2. Consider adding comments in the Read function of local_fs.go and the read function of s3_fs.go to explain the reason for subtracting the IO duration from the CPU stats. This will make the code more self-explanatory and easier to maintain.

  3. Consider adding comments in mysql_cmd_executor.go to explain the reason for modifying the calculation of CPU stats and how it affects the final result. This will help other developers understand the code and its impact on the CPU stats.

  4. Consider adding comments in stats_array.go to explain the purpose and usage of the new methods (AddS3AccessTimeConsumption and AddDiskAccessTimeConsumption). This will make the code more understandable and maintainable.

Overall, the changes made in this pull request seem reasonable and should improve the accuracy of the CPU stats. However, adding comments to explain the code and its intentions will make it easier for other developers to understand and maintain the codebase.

@mergify mergify bot merged commit 4236bf7 into matrixorigin:1.1-dev Jan 10, 2024
17 of 18 checks passed
@Morranto Morranto deleted the 1.1-dev branch January 25, 2024 07:21
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 kind/enhancement size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants