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

sync_to_1.1: defer on time.since not work #14029

Merged

Conversation

gouhongshen
Copy link
Contributor

@gouhongshen gouhongshen commented Jan 5, 2024

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #13983

What this PR does / why we need it:

fix the time.Since not deferred

@gouhongshen gouhongshen requested a review from XuPeng-SH as a code owner January 5, 2024 04:00
@matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Jan 5, 2024
@mergify mergify bot requested a review from sukki37 January 5, 2024 04:01
@gouhongshen gouhongshen mentioned this pull request Jan 5, 2024
7 tasks
@mergify mergify bot added the kind/bug Something isn't working label Jan 5, 2024
@matrix-meow
Copy link
Contributor

@gouhongshen Thanks for your contributions!

Title: sync_to_1.1: defer on time.since not work

Body: This pull request fixes the issue #13983. The problem is that the time.Since function is not being deferred properly. This PR adds a defer statement to ensure that the v2.TaskStorageUsageReqDurationHistogram is observed correctly.

Changes:

  • In handle.go, on line 1272, a defer statement is added to observe the duration of the task storage usage request.

Review:

Overall, the changes made in this pull request seem to address the issue correctly. The addition of the defer statement ensures that the v2.TaskStorageUsageReqDurationHistogram is observed properly. However, there are a few suggestions to optimize the changes:

  1. Naming: The variable start could be renamed to something more descriptive, such as requestStartTime, to improve code readability.

  2. Error Handling: It would be beneficial to handle any potential errors that may occur when observing the duration. This can be done by adding a recover statement within the defer function to catch and log any panics.

  3. Documentation: It would be helpful to add a comment explaining the purpose of the defer statement and its relationship to the time.Since function.

Security Concerns: There don't appear to be any security concerns in this pull request.

Overall, the changes made in this pull request are appropriate and address the issue effectively. The suggestions provided aim to improve code readability, error handling, and documentation.

mergify bot pushed a commit that referenced this pull request Jan 5, 2024
fix the time.Since not deferred


cherry-pick to 1.1: #14029

Approved by: @XuPeng-SH
@mergify mergify bot merged commit c718a94 into matrixorigin:1.1-dev Jan 6, 2024
17 of 18 checks passed
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/XS Denotes a PR that changes [1, 9] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants