sink: capture schema registry failure - #5712
Conversation
ti-chi-bot
Bot
added
release-note
📝 WalkthroughWalkthroughSchema Registry requests now have bounded timeouts and improved retry termination. HTTP 500 responses include status details, unit tests cover registration failures, and an integration test verifies Avro changefeeds enter warning status when schema registration fails. ChangesSchema Registry Error Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/sink/codec/avro/confluent_schema_registry_test.go`:
- Around line 139-141: Update the test around manager.Register to assert that
the returned Avro schema API error includes the HTTP status detail “status 500”,
while preserving the existing ErrAvroSchemaAPIError and zero schema ID
assertions.
In `@tests/integration_tests/avro_schema_registry_error/run.sh`:
- Around line 130-131: Update the two check_changefeed_status assertions in the
avro schema registry error test to enforce a short explicit deadline for the
warning transition instead of allowing all MAX_RETRIES attempts. Keep validating
both avro_changefeed_id and debezium_changefeed_id reach the expected “register
schema failed with status 500” warning within that bounded interval.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fdd785a6-9d62-4d74-8d9a-c40e0a5844ea
📒 Files selected for processing (5)
pkg/sink/codec/avro/confluent_schema_registry.gopkg/sink/codec/avro/confluent_schema_registry_test.gopkg/sink/codec/avro/mock_schema_registry.gotests/integration_tests/avro_schema_registry_error/run.shtests/integration_tests/run_light_it_in_ci.sh
| schemaID, err := manager.Register(ctx, "server-error", codec.Schema()) | ||
| require.ErrorIs(t, err, errors.ErrAvroSchemaAPIError) | ||
| require.Zero(t, schemaID.confluentSchemaID) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the HTTP status detail.
This test passes with the old status-less error, so it does not protect the new clear-error contract. Assert that the returned error includes status 500.
Proposed test assertion
schemaID, err := manager.Register(ctx, "server-error", codec.Schema())
require.ErrorIs(t, err, errors.ErrAvroSchemaAPIError)
+require.ErrorContains(t, err, "status 500")
require.Zero(t, schemaID.confluentSchemaID)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| schemaID, err := manager.Register(ctx, "server-error", codec.Schema()) | |
| require.ErrorIs(t, err, errors.ErrAvroSchemaAPIError) | |
| require.Zero(t, schemaID.confluentSchemaID) | |
| schemaID, err := manager.Register(ctx, "server-error", codec.Schema()) | |
| require.ErrorIs(t, err, errors.ErrAvroSchemaAPIError) | |
| require.ErrorContains(t, err, "status 500") | |
| require.Zero(t, schemaID.confluentSchemaID) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/sink/codec/avro/confluent_schema_registry_test.go` around lines 139 -
141, Update the test around manager.Register to assert that the returned Avro
schema API error includes the HTTP status detail “status 500”, while preserving
the existing ErrAvroSchemaAPIError and zero schema ID assertions.
| ensure "$MAX_RETRIES" "check_changefeed_status '127.0.0.1:8300' '$avro_changefeed_id' 'warning' 'last_warning' 'register schema failed with status 500'" | ||
| ensure "$MAX_RETRIES" "check_changefeed_status '127.0.0.1:8300' '$debezium_changefeed_id' 'warning' 'last_warning' 'register schema failed with status 500'" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert a bounded health-transition time.
These ensure calls allow the warning to appear on any of 20 retries, so a regression that leaves a changefeed healthy for most of that window still passes. Add a short, explicit deadline consistent with the required immediate transition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration_tests/avro_schema_registry_error/run.sh` around lines 130 -
131, Update the two check_changefeed_status assertions in the avro schema
registry error test to enforce a short explicit deadline for the warning
transition instead of allowing all MAX_RETRIES attempts. Keep validating both
avro_changefeed_id and debezium_changefeed_id reach the expected “register
schema failed with status 500” warning within that bounded interval.
wk989898
added
the
needs-cherry-pick-release-8.5
|
/test all |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 3AceShowHand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
2 participants
What problem does this PR solve?
Issue Number: close #5653
What is changed and how it works?
Before this change, when Schema Registry returned 500, TiCDC could keep retrying inside the Avro schema registry helper and the changefeed could remain healthy-looking.
Now, when schema registration gets a 500, TiCDC returns the error immediately. The encoder error flows to the dispatcher manager, then to the maintainer/coordinator error path. The changefeed is reported as warning, with the registry error visible in last_warning.
Check List
Tests
With this pr, changefeed will be in warning status
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note
Summary by CodeRabbit
Bug Fixes
Tests