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

Return useful error when encountering unknown kafka API key #1477

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

rukai
Copy link
Member

@rukai rukai commented Feb 14, 2024

We were loading the api key twice.
The first time we panicked if it was an unknown key.
And then the second time we gracefully returned an error.
This is a bit silly since we never hit the graceful error and always panic.

This PR fixes this so that we only load the api_key once and we perform graceful error handling that one time.

Copy link

1 benchmark regressed. 0 benchmark improved. Please check the benchmark workflow logs for full details: https://github.com/shotover/shotover-proxy/actions/runs/7895482233

                        Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) low mild
kafka_codec/encode_request_produce
                        time:   [1.1065 µs 1.1334 µs 1.1577 µs]
                        change: [+27.992% +34.195% +42.213%] (p = 0.00 < 0.05)
                        Performance has regressed.

@rukai rukai enabled auto-merge (squash) February 14, 2024 12:11
Copy link

0 benchmark regressed. 1 benchmark improved. Please check the benchmark workflow logs for full details: https://github.com/shotover/shotover-proxy/actions/runs/7900968733

  2 (2.00%) low mild
  3 (3.00%) high mild
  1 (1.00%) high severe
kafka_codec/encode_request_produce
                        time:   [656.46 ns 669.73 ns 682.22 ns]
                        change: [-27.036% -25.362% -23.539%] (p = 0.00 < 0.05)
                        Performance has improved.

@rukai rukai merged commit f846d40 into shotover:main Feb 14, 2024
40 checks passed
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.

3 participants