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: remove druid datasource from the config #19770

Merged
merged 7 commits into from
May 4, 2022

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Apr 18, 2022

In #19262 I added to the UPDATING.md file that the native druid datasource was going to be deprecated in 2.0. After further consideration, I think it will also be more explicit and better for semver to actually remove the config in this major release.

TESTING INSTRUCTIONS

Native Druid NoSQL tests need to be removed.

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

@superset-github-bot superset-github-bot bot added the Superset-Community-Partners Preset community partner program participants label Apr 18, 2022
@eschutho eschutho marked this pull request as draft April 18, 2022 23:50
@eschutho eschutho added risk:breaking-change Issues or PRs that will introduce breaking changes v2.0 and removed Superset-Community-Partners Preset community partner program participants labels Apr 18, 2022
@eschutho eschutho changed the title remove druid datasource from the config chore: remove druid datasource from the config Apr 18, 2022
@codecov
Copy link

codecov bot commented Apr 18, 2022

Codecov Report

Merging #19770 (386580d) into master (fdf48c6) will decrease coverage by 0.27%.
The diff coverage is 70.00%.

@@            Coverage Diff             @@
##           master   #19770      +/-   ##
==========================================
- Coverage   66.54%   66.27%   -0.28%     
==========================================
  Files        1714     1712       -2     
  Lines       65102    63964    -1138     
  Branches     6725     6725              
==========================================
- Hits        43321    42389     -932     
+ Misses      20069    19863     -206     
  Partials     1712     1712              
Flag Coverage Δ
hive 53.65% <60.00%> (+0.75%) ⬆️
mysql 81.98% <70.00%> (+0.01%) ⬆️
postgres 82.02% <70.00%> (+<0.01%) ⬆️
presto 53.50% <60.00%> (+0.74%) ⬆️
python 82.45% <70.00%> (+0.01%) ⬆️
sqlite 81.77% <70.00%> (+<0.01%) ⬆️
unit 48.63% <60.00%> (+0.57%) ⬆️

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

Impacted Files Coverage Δ
...tend/src/visualizations/FilterBox/controlPanel.jsx 37.50% <ø> (ø)
superset/cli/update.py 0.00% <ø> (ø)
superset/config.py 91.33% <ø> (-0.09%) ⬇️
superset/models/dashboard.py 75.96% <ø> (+1.06%) ⬆️
superset/utils/dict_import_export.py 77.27% <ø> (+2.27%) ⬆️
superset/views/core.py 77.37% <ø> (+0.29%) ⬆️
superset/datasets/commands/importers/v0.py 91.47% <50.00%> (-2.15%) ⬇️
superset/dashboards/commands/importers/v0.py 91.87% <100.00%> (+1.93%) ⬆️
superset/initialization/__init__.py 91.39% <100.00%> (-0.33%) ⬇️
superset/security/manager.py 95.45% <100.00%> (+0.18%) ⬆️
... and 10 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 fdf48c6...386580d. Read the comment docs.

@eschutho eschutho force-pushed the elizabeth/remove-druid-config branch 2 times, most recently from ffc495b to bc14df8 Compare April 19, 2022 17:48
@eschutho eschutho marked this pull request as ready for review April 19, 2022 17:51
@eschutho eschutho force-pushed the elizabeth/remove-druid-config branch 4 times, most recently from 5e718ef to 0c6597f Compare April 20, 2022 21:56
Copy link
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

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

I thought for Superset 2.0 we were just going to mark this as deprecated and actually remove it in Superset 3.0? This change could be seen as too aggressive for some institutions and ideally we should strive not to unnecessarily churn users.

UPDATING.md Show resolved Hide resolved
@eschutho
Copy link
Member Author

eschutho commented Apr 20, 2022

I thought for Superset 2.0 we were just going to mark this as deprecated and actually remove it in Superset 3.0? This change could be seen as too aggressive for some institutions and ideally we should strive not to unnecessarily churn users.

I think the original plan was to remove native Druid support for SIP68, and in trying to get ahead of that work, to mark it as "no longer supported" in 2.0. I believe there is likely going to be something coming before 3.0 that will break this functionality. Do you think that we need to give people more time to switch over to Druid Sql? If absolutely necessary, we can try not to break native Druid as part of the SIP68 work, but I think it's going to be complicated. Or we just hold off on finishing the SIP68 work until 3.0. cc @betodealmeida

I figured the only reason that we didn't actually remove the functionality in 2.0 was the time it would take to write this PR.

@villebro
Copy link
Member

I thought for Superset 2.0 we were just going to mark this as deprecated and actually remove it in Superset 3.0? This change could be seen as too aggressive for some institutions and ideally we should strive not to unnecessarily churn users.

@john-bodley I understand the concern here, but at the same time the community has had ample time to prepare for this (link to the SIP #6032 ). Maybe we could float this via the dev list - if there's strong opposition maybe we can postpone until 3.0, but otherwise just rip the bandaid off in 2.0?

My personal opinion: the native Druid connector is already missing some key functionality, so I'm actually surprised if it's even possible to use it in a production setting anymore. So I'd vote for removing it in 2.0 or at least making it abundantly clear that it's no longer maintained and may stop functioning all together at any given point after 2.0.

@eschutho eschutho force-pushed the elizabeth/remove-druid-config branch from 0c6597f to 5bf370c Compare April 21, 2022 18:54
@eschutho eschutho requested a review from nytai April 22, 2022 23:24
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 is awesome! I left a few minor comments.

superset/datasets/commands/importers/v0.py Outdated Show resolved Hide resolved
superset/datasets/commands/importers/v0.py Outdated Show resolved Hide resolved
superset/datasets/commands/importers/v0.py Outdated Show resolved Hide resolved
@eschutho eschutho force-pushed the elizabeth/remove-druid-config branch from c707c11 to 386580d Compare May 4, 2022 17:27
@eschutho eschutho merged commit 060b5c0 into apache:master May 4, 2022
@eschutho eschutho deleted the elizabeth/remove-druid-config branch May 4, 2022 19:49
hughhhh pushed a commit to hve-labs/superset that referenced this pull request May 11, 2022
* remove druid datasource from the config

* remove config related references to DruidDatasource

* Update __init__.py

* Update __init__.py

* Update manager.py

* remove config related references to DruidDatasource

* raise if instance type is not valid
philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022
* remove druid datasource from the config

* remove config related references to DruidDatasource

* Update __init__.py

* Update __init__.py

* Update manager.py

* remove config related references to DruidDatasource

* raise if instance type is not valid
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels preset-io risk:breaking-change Issues or PRs that will introduce breaking changes size/XXL 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants