-
Notifications
You must be signed in to change notification settings - Fork 45
Do not use fivetran synced dates older than or equal March 9 2024 #4866
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
Do not use fivetran synced dates older than or equal March 9 2024 #4866
Conversation
Signed-off-by: Lukasz Gryglicki <[email protected]> Assisted by [OpenAI](https://platform.openai.com/) Assisted by [GitHub Copilot](https://github.com/features/copilot)
WalkthroughA cutoff-based gating mechanism is introduced for Fivetran-synced timestamps in the backfill process. A new constant and helper function determine if timestamps precede a cutoff date; the Snowflake pass conditionally skips using _FIVETRAN_SYNCED for created and modified timestamps when before the cutoff to maintain consistent ordering. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cla-backend-go/cmd/signatures_timestamp_backfill/main.go (1)
40-41: Add documentation for the cutoff date.The constant
fivetranCutoffDateis set to March 9, 2024, but there's no explanation for why this specific date is significant. Consider adding a comment explaining the reason for this cutoff (e.g., data quality issues, migration event, system change, etc.) to help future maintainers understand the context.Apply this diff to add documentation:
- // cutoff date for _FIVETRAN_SYNCED usage - fivetranCutoffDate = "2024-03-09T00:00:00Z" + // cutoff date for _FIVETRAN_SYNCED usage + // Fivetran synced timestamps on or before this date are unreliable and should not be used. + // TODO: Add ticket reference or explanation for why March 9, 2024 is the cutoff + fivetranCutoffDate = "2024-03-09T00:00:00Z"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
cla-backend-go/cmd/signatures_timestamp_backfill/main.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (go)
- GitHub Check: build-test-lint
- GitHub Check: cypress-functional
🔇 Additional comments (2)
cla-backend-go/cmd/signatures_timestamp_backfill/main.go (2)
552-576: LGTM! Cutoff gating correctly implemented.The Snowflake pass now correctly checks if
_FIVETRAN_SYNCEDtimestamps fall on or before the cutoff date and skips them accordingly. The logic ensures:
- Created timestamps only use Fivetran data when
!skipFivetran(line 559)- Modified timestamps fall back to other sources when
skipFivetranis true (line 574)- Appropriate debug logging is added for visibility (line 555)
The fallback behavior for both created and modified timestamps is preserved.
1079-1090: Consider the trade-off of conservative error handling.The function returns
true(skip Fivetran data) when parsing fails (lines 1083, 1087). This is a conservative approach that errs on the side of caution, but it means that:
- Transient parse errors will cause valid Fivetran timestamps to be skipped
- Unexpected timestamp formats will be rejected even if they're after the cutoff
This might be the intended behavior for data integrity, but consider whether logging these parse failures as warnings would help identify systematic issues with timestamp formats.
The core comparison logic at line 1089 (
!t.After(cutoff)) is correct and properly implements "skip if timestamp ≤ cutoff".Do you want to add warning logs for parse failures to help identify potential data quality issues?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request adds logic to exclude Fivetran synced timestamps that are on or before March 9, 2024, from being used in the signature timestamp backfill process. This prevents potentially unreliable historical Fivetran data from affecting timestamp corrections.
Key Changes:
- Added a cutoff date constant (
fivetranCutoffDate = "2024-03-09T00:00:00Z") - Implemented
isFivetranBeforeCutoff()function to check if a timestamp should be excluded - Modified the
snowflakeFix()function to skip Fivetran timestamps when they fall on or before the cutoff date
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cc @mlehotskylf @ahmedomosanya
Signed-off-by: Lukasz Gryglicki [email protected]
Assisted by OpenAI
Assisted by GitHub Copilot