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

chore: skip SIP-68 shadow writing for LTS branches #19636

Closed
wants to merge 1 commit into from

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Apr 9, 2022

SUMMARY

Change SIP-68 db migrations to no-op and stop shadow-writing datasets to new models.

We should not include the shadow writing logic to official releases as it increases computation costs yet contributes nothing to end user experience (for now). Given the original migration is expensive and we likely need to redo it anyway, it's the best to skip it for now.

This PR should only be cherry-picked into the LTS branches such as 1.5 and 2.0. Once #19421 is ready and the new dataset models started to replace the old models, we'll just re-cut the release from master and ignore this PR.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@codecov
Copy link

codecov bot commented Apr 9, 2022

Codecov Report

Merging #19636 (69bc743) into 1.5 (419316e) will increase coverage by 15.02%.
The diff coverage is 90.45%.

❗ Current head 69bc743 differs from pull request most recent head d6e60e0. Consider uploading reports for the commit d6e60e0 to get more accurate results

@@             Coverage Diff             @@
##              1.5   #19636       +/-   ##
===========================================
+ Coverage   51.64%   66.66%   +15.02%     
===========================================
  Files        1686     1690        +4     
  Lines       65205    65146       -59     
  Branches     6531     6550       +19     
===========================================
+ Hits        33676    43432     +9756     
+ Misses      29861    20039     -9822     
- Partials     1668     1675        +7     
Flag Coverage Δ
hive 52.32% <39.88%> (?)
mysql 82.03% <94.38%> (?)
postgres 82.08% <94.38%> (?)
presto ?
python 82.38% <94.38%> (+30.06%) ⬆️
sqlite 81.83% <94.38%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...omponents/ColumnConfigControl/ColumnConfigItem.tsx 0.00% <ø> (ø)
...superset-ui-core/src/query/types/PostProcessing.ts 100.00% <ø> (ø)
...in-chart-echarts/src/MixedTimeseries/buildQuery.ts 0.00% <ø> (ø)
superset-frontend/src/SqlLab/actions/sqlLab.js 60.39% <ø> (ø)
...ntend/src/SqlLab/components/QueryHistory/index.tsx 66.66% <ø> (ø)
...dashboard/components/SliceHeaderControls/index.tsx 66.66% <ø> (ø)
...d/src/explore/components/DatasourcePanel/index.tsx 69.35% <ø> (ø)
.../src/explore/components/controls/SliderControl.tsx 0.00% <0.00%> (ø)
superset-frontend/src/utils/urlUtils.ts 36.73% <ø> (ø)
superset/connectors/druid/models.py 82.99% <ø> (+56.57%) ⬆️
... and 375 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 419316e...d6e60e0. Read the comment docs.

@ktmud
Copy link
Member Author

ktmud commented Apr 9, 2022

CI failed but the base branch is showing the same error: https://github.com/apache/superset/runs/5886127859?check_suite_focus=true

I'll leave it as is.

@villebro
Copy link
Member

CI failed but the base branch is showing the same error: https://github.com/apache/superset/runs/5886127859?check_suite_focus=true

I'll leave it as is.

Ugh, I thought I fixed it, but maybe I forgot to commit the fix.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (FYI I resolved the build errors on the 1.5 branch) - However, leaving to @betodealmeida to approve as he knows SIP-68 far better than me.

@villebro
Copy link
Member

@ktmud this PR appears to have a few linting issues (the underlying branch is now green). We're ready to cut 1.5.0rc3 (there's two fixes in it), but going to wait to get this one in.

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Thanks, @ktmud!

@ktmud
Copy link
Member Author

ktmud commented Apr 12, 2022

@ktmud this PR appears to have a few linting issues (the underlying branch is now green). We're ready to cut 1.5.0rc3 (there's two fixes in it), but going to wait to get this one in.

Let me fix those

@ktmud
Copy link
Member Author

ktmud commented Apr 12, 2022

@villebro The helm chart failure seems to be related to Helm chart being updated to a new version in master. The base branch didn't trigger an error because the lint job only runs on pull requests. Should we create a new PR to fix it first?

@ktmud
Copy link
Member Author

ktmud commented Apr 12, 2022

It seems the Helm Chart has the content as master. I'm not familiar with how Helm works and not sure whether I should bump the version in this branch. Maybe leave as it and just cherry-pick without merge?

@villebro
Copy link
Member

It seems the Helm Chart has the content as master. I'm not familiar with how Helm works and not sure whether I should bump the version in this branch. Maybe leave as it and just cherry-pick without merge?

This is really confusing. Sounds good, I'll just pick this in as-is 👍

@villebro villebro removed the v1.5 label Apr 15, 2022
@villebro
Copy link
Member

Removed v1.5 label to not confuse cherrytree when baking rc3.

@michael-s-molina
Copy link
Member

@ktmud Can you check CI here? 2.0 is being assembled right now.

@ktmud
Copy link
Member Author

ktmud commented Jun 27, 2022

Given that #19421 is already merged and already significantly improved the migration performance, we can probably skip this for 2.0 to reduce operational load---otherwise we'd have to make the new migration a no-op and add it back in later releases, too.

@michael-s-molina
Copy link
Member

Should we close this?

@ktmud ktmud closed this Jun 28, 2022
@john-bodley john-bodley deleted the skip-sip-68 branch February 17, 2023 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants