Add metrics/DLQ record for records getting silently dropped with a warning in reverse replication #3200
Conversation
Summary of ChangesHello @aasthabharill, 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 focuses on enhancing the observability and understanding of record processing within the reverse replication pipeline. It introduces a series of detailed comments to existing logging statements across several core components. These comments serve to identify and categorize various error and warning conditions, explicitly noting where records might be silently dropped, where exceptions are handled (or not), and how different issues interact with the Dead Letter Queue (DLQ) and retry mechanisms. This work lays the groundwork for future improvements in error handling, metrics, and DLQ integration by clearly documenting current behaviors and potential points of failure. 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3200 +/- ##
============================================
+ Coverage 50.80% 50.87% +0.06%
- Complexity 5124 5511 +387
============================================
Files 976 976
Lines 60058 60098 +40
Branches 6569 6572 +3
============================================
+ Hits 30511 30573 +62
+ Misses 27406 27370 -36
- Partials 2141 2155 +14
🚀 New features to boost your workflow:
|
| failsafeElement.setErrorMessage(record.getErrorMessage()); | ||
| c.output(failsafeElement); | ||
| } catch (Exception e) { | ||
| LOG.error( |
There was a problem hiding this comment.
Should this be InvalidDMLGenerationException instead of a catch all?
| LOG.warn("The column {} was not found in input record", spannerColName); | ||
| LOG.warn( | ||
| "The column {} was not found in input record", | ||
| spannerColName); // aastha warning - problematic - caught by above function |
There was a problem hiding this comment.
[nit] please consider rewording the comment. Likewise for other places having aastha warning -
shreyakhajanchi
left a comment
shreyakhajanchi
left a comment
There was a problem hiding this comment.
Can you test for cases where if the schema is fixed and the severe dlq errors classified as part of the current pr are replayed then they get successfully migrated?
| } catch (IOException e) { | ||
| LOG.error("Issue parsing JSON record {}. Unable to continue.", line, e); | ||
| throw new RuntimeException(e); | ||
| LOG.error("Issue parsing JSON record {}. Skipping record.", line, e); |
There was a problem hiding this comment.
This code is common across other templates, non spanner templates as well. Why do we need to make the change here?
| public class MySQLDMLGenerator implements IDMLGenerator { | ||
| private static final Logger LOG = LoggerFactory.getLogger(MySQLDMLGenerator.class); | ||
|
|
||
| public DMLGeneratorResponse getDMLStatement(DMLGeneratorRequest dmlGeneratorRequest) { |
There was a problem hiding this comment.
handle the log in InputProcessor as well:
LOG.warn("DML statement is empty for table: " + tableName);
| c.output(failsafeElement); | ||
| } catch (Exception e) { | ||
| LOG.error( | ||
| "Failed to parse ChangeStreamErrorRecord from DLQ. Dropping record: {}", c.element(), e); |
There was a problem hiding this comment.
We should not drop the record. Have we encountered any case where the current code was not working or trying indefinitely as mentioned in the description?
3 participants
https://b.corp.google.com/issues/432145898
Summary
This PR addresses critical issues in the SpannerToSourceDb pipeline where certain error conditions were being silently dropped or causing inefficient worker retries. A thorough investigation of LOG warnings and exceptions revealed that invalid DML generation and specific JSON parsing failures were not being handled robustly.
Changes
1. DML Generation Integrity
instead of returning an empty string.
It is then caught in SourceWriterFn, correctly unwrapped and identified by SpannerToSourceDbExceptionClassifier, and finally routed to the DLQ with a PERMANENT_ERROR_TAG to ensure visibility and prevent data loss.
2. Robust JSON Parsing & Retry Prevention
ConvertDlqRecordToTrimmedShardedDataChangeRecordFn lacked sufficient error handling for malformed input, allowing parsing exceptions to bubble up unchecked.