feat(spanner-to-sourcedb): Classify NOT_FOUND as retryable to support interleaved table replication - #4045
Conversation
…leaved table replication
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness of the Spanner-to-SourceDb replication pipeline by enabling retries for Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes NOT_FOUND errors retryable in the Spanner-to-SourceDB pipeline and adds an integration test (spannerToSpannerInterleavedOutOfOrderTest) to verify out-of-order replication of interleaved tables. Feedback on these changes suggests recursively traversing the exception chain to distinguish between row-level and schema-level NOT_FOUND errors to prevent infinite retries on permanent schema mismatches. Additionally, it is recommended to replace the hardcoded Thread.sleep(5000) in the integration test with a polling mechanism (such as checking the GCS DLQ) to prevent potential race conditions and test flakiness.
| ErrorCode.INVALID_ARGUMENT, | ||
| ErrorCode.NOT_FOUND, | ||
| ErrorCode.FAILED_PRECONDITION, |
There was a problem hiding this comment.
When classifying exceptions, we should recursively traverse the exception chain (using getCause()) to inspect both the outer exception and all nested causes, rather than only checking the immediate cause, to robustly handle both wrapped and unwrapped exceptions.
Additionally, classifying all NOT_FOUND errors as retryable can lead to unnecessary retries and resource waste for permanent schema-level errors (such as "Table not found" or "Column not found"). These schema mismatches are permanent and will never succeed upon retrying.
To prevent this, we should distinguish between row-level NOT_FOUND (e.g., missing parent row in interleaved tables) and schema-level NOT_FOUND by traversing the exception chain and inspecting the messages.
References
- When classifying exceptions, recursively traverse the exception chain (using
getCause()) to inspect both the outer exception and all nested causes, rather than only checking the immediate cause, to robustly handle both wrapped and unwrapped exceptions.
There was a problem hiding this comment.
I'd like to push back for the following reasons:
-
Traversing the chain: Writes occur within Spanner's runInTransaction callback, which wraps the immediate exception in a SpannerException. Checking the immediate getCause() should be sufficient; recursion is unnecessary complexity.
-
Parsing error messages: Distinguishing schema vs. row-level NOT_FOUND errors requires string-matching error messages. This is a brittle as message strings are not a stable API contract.
-
Operational resilience: In CDC, treating a missing table as "permanent" forces data into a DLQ. Keeping it retryable should give the pipeline the opportunity to "self-heal" once an operator applies the missing DDL.
| .to("Updated Child") | ||
| .build()); | ||
|
|
||
| Thread.sleep(5000); |
There was a problem hiding this comment.
Using a hardcoded Thread.sleep(5000) introduces a potential race condition and can make the test flaky or slow.
If the pipeline is slow to process the child update, the 5-second sleep might not be enough, and the parent row might be written to the source before the child update is attempted at the destination. If that happens, the child update will succeed on its first attempt without ever triggering the NOT_FOUND error or exercising the retry logic, resulting in a false positive test.
Instead of a fixed sleep, consider polling the DLQ directory in GCS using gcsResourceManager to verify that a retryable error has actually been logged before proceeding to restore the parent row.
There was a problem hiding this comment.
I'd like to keep the Thread.sleep because I'd expect polling the DLQ would drastically slow down the test. In a streaming pipeline, writing to a GCS DLQ requires windowing, meaning we would have to wait a minute or more just for the file to be emitted.
Sleep seems to be used across of other our integration tests and 5secs provides enough time to propagte recordsin an isolated environment without unnecessarily inflating the test duration.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4045 +/- ##
============================================
+ Coverage 55.74% 55.76% +0.02%
+ Complexity 7240 6799 -441
============================================
Files 1124 1126 +2
Lines 68476 68557 +81
Branches 7726 7734 +8
============================================
+ Hits 38173 38232 +59
- Misses 27826 27845 +19
- Partials 2477 2480 +3
🚀 New features to boost your workflow:
|
| child_id INT64 NOT NULL, | ||
| description STRING(25) | ||
| ) PRIMARY KEY(id, child_id), | ||
| INTERLEAVE IN PARENT Parent ON DELETE CASCADE; |
There was a problem hiding this comment.
This should be on delete no action.
When you on delete cascade, the child will get removed as well.
There was a problem hiding this comment.
fixed - reworked schema and the actual test
| .set("description") | ||
| .to("Parent row") | ||
| .build()); | ||
| spannerResourceManager.write( |
There was a problem hiding this comment.
The first part of the test does not accomplish much. We can just do the 2nd part, which is create the child first and parent 2nd, and that will assert the DLQ flow
There was a problem hiding this comment.
fixed - reworked schema and the actual test
2 participants
This PR classifies
NOT_FOUNDdatabase errors as retryable in the Spanner-to-SourceDb template.When replicating interleaved tables, child record mutations can be delivered before their parent records. Currently, this results in a non-retryable
NOT_FOUNDerror, causing replication to fail.By classifying
NOT_FOUNDas retryable, the pipeline will retry the child write until the parent record is successfully replicated.Changes:
ErrorCode.NOT_FOUNDfrom the non-retryable errors list inSpannerToSourceDbExceptionClassifier.spannerToSpannerInterleavedOutOfOrderTestinSpannerToSpannerITto simulate out-of-order delivery (deleting a parent on destination, writing a child update on source, then writing the parent) and verified that replication succeeds after a retry.