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

[cherry-pick] pr #13909 fix some unhandled error and redundant codes #13910

Merged
merged 3 commits into from Dec 30, 2023

Conversation

m-schen
Copy link
Contributor

@m-schen m-schen commented Dec 28, 2023

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #13908

What this PR does / why we need it:

some error was unhandled and it may cause hung.
removed some unused codes like redundant type conversion.

@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Dec 28, 2023
@mergify mergify bot requested a review from sukki37 December 28, 2023 10:00
@mergify mergify bot added the kind/bug Something isn't working label Dec 28, 2023
@matrix-meow
Copy link
Contributor

@m-schen Thanks for your contributions!

Review:

Title: [cherry-pick] pr #13909 fix some unhandled error and redundant codes

Body:

Changes:

  • In pkg/sql/compile/compile.go, the code now checks for errors when submitting tasks to the ants pool and handles the error by sending it to the errC channel and calling wg.Done().
  • In pkg/sql/compile/ddl.go, the code removes unnecessary type conversion when appending addIndex to newCt.Cts.
  • In pkg/sql/compile/ddl.go, the code now assigns the result of dbSource.Truncate to a variable err and checks for errors.
  • In pkg/sql/compile/ddl.go, the code removes unnecessary type conversion when comparing startNum with preStartWith.
  • In pkg/sql/compile/fuzzyCheck.go, the code now checks for errors when writing to the all buffer and returns the error if any.
  • In pkg/sql/compile/runtime_filter.go, the code assigns filter.Data directly to Zm without unnecessary type conversion.
  • In pkg/sql/compile/scope.go, the code now checks for errors when submitting tasks to the ants pool and handles the error by sending it to the errChan channel and calling wg.Done().
  • In pkg/sql/compile/util.go, the code now calls cancelFunc() after assigning index_id to handle the context cancellation properly.

Overall, the changes made in this pull request address the unhandled errors and remove redundant code. However, there are a few suggestions for further improvement:

  1. Error handling in pkg/sql/compile/compile.go and pkg/sql/compile/scope.go:

    • Instead of sending the error to the errC and errChan channels and calling wg.Done() separately, it would be more efficient to use a defer statement to handle the error and call wg.Done() in a single place.
    • Example:
      errSubmit := ants.Submit(func() {
          defer func() {
              if e := recover(); e != nil {
                  err := moerr.ConvertPanicError(c.ctx, e)
                  errC <- err
              }
              wg.Done()
          }()
          // Task logic here
      })
      
  2. Redundant type conversion in pkg/sql/compile/ddl.go:

    • The type conversion in the comparison startNum < preStartWith is unnecessary since startNum and preStartWith are already of the same type. The comparison can be simplified to startNum < preStartWith.
  3. Error handling in pkg/sql/compile/fuzzyCheck.go:

    • Instead of returning the error immediately when writing to the all buffer, it would be better to accumulate the errors in a slice and return them all at once. This way, the caller can handle multiple errors if they occur.
  4. Context cancellation in pkg/sql/compile/util.go:

    • The cancelFunc() should be called before returning from the function to ensure proper cancellation of the context. It can be moved outside the if err != nil block.

These suggestions will help improve the code quality and optimize the changes made in the pull request.

@m-schen
Copy link
Contributor Author

m-schen commented Dec 28, 2023

chatgpt好像越活越倒退了。。

1,2,4点就是我改的内容。
第三点纯纯乱讲 😠😠

@mergify mergify bot merged commit f435599 into matrixorigin:1.1-dev Dec 30, 2023
17 of 18 checks passed
@m-schen m-schen deleted the 1.1-fix-h branch January 2, 2024 03:39
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

4 participants