You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/04/18 23:50:21 UTC
[GitHub] [superset] eschutho opened a new pull request, #19770: remove druid datasource from the config
eschutho opened a new pull request, #19770:
URL: https://github.com/apache/superset/pull/19770
In https://github.com/apache/superset/pull/19262/files 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
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho merged pull request #19770: chore: remove druid datasource from the config
Posted by GitBox <gi...@apache.org>.
eschutho merged PR #19770:
URL: https://github.com/apache/superset/pull/19770
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #19770: chore: remove druid datasource from the config
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19770:
URL: https://github.com/apache/superset/pull/19770#issuecomment-1101865040
# [Codecov](https://codecov.io/gh/apache/superset/pull/19770?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19770](https://codecov.io/gh/apache/superset/pull/19770?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4628feb) into [master](https://codecov.io/gh/apache/superset/commit/9a9e3b6e3bafd9a0d58b2f49404c19b31d2bc48a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9a9e3b6) will **decrease** coverage by `12.82%`.
> The diff coverage is `63.06%`.
> :exclamation: Current head 4628feb differs from pull request most recent head a908bda. Consider uploading reports for the commit a908bda to get more accurate results
```diff
@@ Coverage Diff @@
## master #19770 +/- ##
===========================================
- Coverage 66.49% 53.67% -12.83%
===========================================
Files 1681 1687 +6
Lines 64370 64620 +250
Branches 6583 6646 +63
===========================================
- Hits 42803 34682 -8121
- Misses 19886 28238 +8352
- Partials 1681 1700 +19
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.54% <64.34%> (+0.02%)` | :arrow_up: |
| python | `56.24% <67.82%> (-26.14%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `47.75% <63.47%> (+0.04%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19770?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...chart-controls/src/shared-controls/dndControls.tsx](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9kbmRDb250cm9scy50c3g=) | `35.89% <ø> (ø)` | |
| [...controls/src/shared-controls/emitFilterControl.tsx](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9lbWl0RmlsdGVyQ29udHJvbC50c3g=) | `50.00% <ø> (ø)` | |
| [...et-ui-chart-controls/src/shared-controls/index.tsx](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY2hhcnQtY29udHJvbHMvc3JjL3NoYXJlZC1jb250cm9scy9pbmRleC50c3g=) | `36.19% <ø> (ø)` | |
| [...d/packages/superset-ui-core/src/models/Registry.ts](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvbW9kZWxzL1JlZ2lzdHJ5LnRz) | `100.00% <ø> (ø)` | |
| [...superset-ui-core/src/query/types/PostProcessing.ts](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvcXVlcnkvdHlwZXMvUG9zdFByb2Nlc3NpbmcudHM=) | `100.00% <ø> (ø)` | |
| [...nd/plugins/legacy-preset-chart-deckgl/src/utils.js](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcHJlc2V0LWNoYXJ0LWRlY2tnbC9zcmMvdXRpbHMuanM=) | `0.00% <0.00%> (ø)` | |
| [...gacy-preset-chart-nvd3/src/DistBar/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcHJlc2V0LWNoYXJ0LW52ZDMvc3JjL0Rpc3RCYXIvY29udHJvbFBhbmVsLnRz) | `11.11% <ø> (ø)` | |
| [...nd/plugins/legacy-preset-chart-nvd3/src/NVD3Vis.js](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9sZWdhY3ktcHJlc2V0LWNoYXJ0LW52ZDMvc3JjL05WRDNWaXMuanM=) | `0.00% <0.00%> (ø)` | |
| [...s/plugin-chart-echarts/src/BoxPlot/controlPanel.ts](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvQm94UGxvdC9jb250cm9sUGFuZWwudHM=) | `100.00% <ø> (ø)` | |
| [...ns/plugin-chart-echarts/src/Gauge/controlPanel.tsx](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvR2F1Z2UvY29udHJvbFBhbmVsLnRzeA==) | `100.00% <ø> (ø)` | |
| ... and [376 more](https://codecov.io/gh/apache/superset/pull/19770/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19770?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19770?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [9a9e3b6...a908bda](https://codecov.io/gh/apache/superset/pull/19770?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #19770: chore: remove druid datasource from the config
Posted by GitBox <gi...@apache.org>.
villebro commented on PR #19770:
URL: https://github.com/apache/superset/pull/19770#issuecomment-1104786146
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] eschutho commented on pull request #19770: chore: remove druid datasource from the config
Posted by GitBox <gi...@apache.org>.
eschutho commented on PR #19770:
URL: https://github.com/apache/superset/pull/19770#issuecomment-1104562178
> 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 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] betodealmeida commented on a diff in pull request #19770: chore: remove druid datasource from the config
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on code in PR #19770:
URL: https://github.com/apache/superset/pull/19770#discussion_r861227053
##########
superset/datasets/commands/importers/v0.py:
##########
@@ -97,9 +76,6 @@ def import_dataset(
if isinstance(i_datasource, SqlaTable):
lookup_database = lookup_sqla_database
lookup_datasource = lookup_sqla_table
- else:
- lookup_database = lookup_druid_cluster
- lookup_datasource = lookup_druid_datasource
Review Comment:
Let's leave the `else` and raise an exception inside the block, otherwise we would end with undefined variables in lines 107, 108.
##########
superset/datasets/commands/importers/v0.py:
##########
@@ -152,22 +115,9 @@ def lookup_sqla_column(session: Session, column: TableColumn) -> TableColumn:
)
-def lookup_druid_column(session: Session, column: DruidColumn) -> DruidColumn:
- return (
- session.query(DruidColumn)
- .filter(
- DruidColumn.datasource_id == column.datasource_id,
- DruidColumn.column_name == column.column_name,
- )
- .first()
- )
-
-
def import_column(session: Session, column: BaseColumn) -> BaseColumn:
if isinstance(column, TableColumn):
lookup_column = lookup_sqla_column
- else:
- lookup_column = lookup_druid_column
Review Comment:
Same here.
##########
superset/datasets/commands/importers/v0.py:
##########
@@ -122,22 +98,9 @@ def lookup_sqla_metric(session: Session, metric: SqlMetric) -> SqlMetric:
)
-def lookup_druid_metric(session: Session, metric: DruidMetric) -> DruidMetric:
- return (
- session.query(DruidMetric)
- .filter(
- DruidMetric.datasource_id == metric.datasource_id,
- DruidMetric.metric_name == metric.metric_name,
- )
- .first()
- )
-
-
def import_metric(session: Session, metric: BaseMetric) -> BaseMetric:
if isinstance(metric, SqlMetric):
lookup_metric = lookup_sqla_metric
- else:
- lookup_metric = lookup_druid_metric
Review Comment:
Same here:
```python
else:
raise Exception(f"Invalid metric type: {metric}")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #19770: chore: remove druid datasource from the config
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #19770:
URL: https://github.com/apache/superset/pull/19770#discussion_r854623431
##########
UPDATING.md:
##########
@@ -50,7 +50,7 @@ flag for the legacy datasource editor (DISABLE_LEGACY_DATASOURCE_EDITOR) in conf
- [19017](https://github.com/apache/superset/pull/19017): Removes Python 3.7 support.
- [19142](https://github.com/apache/superset/pull/19142): Changes feature flag for versioned export(VERSIONED_EXPORT) to be true.
- [19107](https://github.com/apache/superset/pull/19107): Feature flag `SQLLAB_BACKEND_PERSISTENCE` is now on by default, which enables persisting SQL Lab tabs in the backend instead of the browser's `localStorage`.
-- [19262](https://github.com/apache/superset/pull/19262): As per SIPs 11 and 68, the native NoSQL Druid connector is deprecated as of 2.0 and will no longer be supported. Druid is still supported through SQLAlchemy via pydruid.
+- [19770](https://github.com/apache/superset/pull/19770): As per SIPs 11 and 68, the native NoSQL Druid connector is deprecated and has been removed. Druid is still supported through SQLAlchemy via pydruid. The config keys `DRUID_IS_ACTIVE` and `DRUID_METADATA_LINKS_ENABLED` have also been removed.
Review Comment:
Nit. Can we place this in reverse chronological order? You'll have to rebased as I've merged a change to `UPDATING.md` to order these.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org