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

refactor: Simplify IsUnimplemented #478

Closed

Conversation

erezrokah
Copy link
Member

@erezrokah erezrokah commented Dec 8, 2022

Summary

We can simplify this using status.Covert.

Also gRPC doesn't wrap errors so I don't think we need the recursion

Kept the recursion as we are the ones doing wrapping of errors in

return nil, fmt.Errorf("failed to call GetTablesForSpec: %w", err)


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@github-actions
Copy link

github-actions bot commented Dec 11, 2022

⏱️ Benchmark results

Comparing with 62692b9

  • DefaultConcurrency-2 resources/s: 11,595 ⬇️ 7.24% decrease vs. 62692b9
  • Glob-2 ns/op: 146 ⬇️ 8.63% decrease vs. 62692b9
  • TablesWithChildrenDefaultConcurrency-2 resources/s: 29,298 ⬇️ 7.67% decrease vs. 62692b9
  • BufferedScanner-2 ns/op: 9.278 ⬇️ 1.23% decrease vs. 62692b9
  • LogReader-2 ns/op: 30.94 ⬆️ 0.65% increase vs. 62692b9

kodiakhq bot pushed a commit that referenced this pull request Dec 24, 2022

Similar to #478.
`status.Convert` calls `FromError` internally.
Some notes:
1. No need to check `s != nil` as `status.Convert` always returns a non `nil` value if `err != nil`
2. `FromError` returns `false` if `err` is not a gRPC error. This can never happen in this case. Also I believe the `failed to call GetProtocolVersion` is wrong as it should be `err is not a gRPC error`. Regardless I don't think we need it

---
@erezrokah
Copy link
Member Author

Closing as stale

@erezrokah erezrokah closed this Jan 18, 2023
@erezrokah erezrokah deleted the refactor/is_unimplemented branch January 18, 2023 10:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants