Fix platform instance support on Druid ingestion

[Rasnar]

If the platform instance was specified in a Druid ingestion, it was always set twice, because it was overrided in the get_identifier, and then added again down the line (see #11639).
This may break existing users that already rely on the broken platform instance implementation, but they could still manually set it to the old pattern in their dhub file.

Checklist

• [ x] The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
• [ x] Links to related issues (if applicable)
  Druid dataset URN is not generated correctly #11639
  Druid ingestion using platform_instance creates a duplicate in the dataset URN #12546
• Tests for the changes have been added/updated (if applicable)
• Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
• For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files with missing lines	Coverage Δ
...ngestion/src/datahub/ingestion/source/sql/druid.py	100.00% <100.00%> (ø)

... and 10 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

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

[sgomezvillamor]

This is restoring the original implementation, from 2021 https://github.com/datahub-project/datahub/pull/2284/files
which was updated on 2022 https://github.com/datahub-project/datahub/pull/3996/files#diff-d2b27025056d73e6bf1508f17c065c63dd4249caacf26de6199aaeaf446eec60

Have we been producing "duplicated" platform instance since then? 🤔

[Rasnar]

Yes, pretty sure that is the case, or something along the way changed.
But you can see that the default SQLAlchemy has no mention of a platform_instance:
https://github.com/datahub-project/datahub/blob/master/metadata-ingestion/src/datahub/ingestion/source/sql/sql_common.py#L576

If you check any other SQL ingestion with the platform_instance support, e.g. mssql, you can see that also there there is no need to specify the platform_instance at this level:
https://github.com/datahub-project/datahub/blob/master/metadata-ingestion/src/datahub/ingestion/source/sql/mssql/source.py#L731

The Druid ingestion is probably not used a lot, because you will other big inconsistencies on the ingestion. For example, the Druid table is always hardcoded to druid/v2/sql because of the sql alchemy implementation that by default will use the URI to compute the table name. This is probably something that should be fixed at some point.

[sgomezvillamor]

Integration tests for druid source were missed; I just added them here #12717
And indeed, the golden file shows the platform instance appears twice in the generated URNs for datasets.

Let's merge PR with integration tests first and then yours. Thanks for raising the issue and the fix.

[Rasnar]

I will wait on #12717 to get merged, and rebase this PR with your changes.
Thanks a lot on the support!

[sgomezvillamor]

PR #12717 merged

[sgomezvillamor]

👍 Thanks for the contrib!

[Rasnar]

One of the builds seems to have failed on a non-related test:

=========================== short test summary info ============================
ERROR tests/integration/kafka-connect/test_kafka_connect.py::test_kafka_connect_ingest - Exception: Timeout reached while waiting on service!
ERROR tests/integration/kafka-connect/test_kafka_connect.py::test_kafka_connect_mongosourceconnect_ingest - Exception: Timeout reached while waiting on service!
ERROR tests/integration/kafka-connect/test_kafka_connect.py::test_kafka_connect_s3sink_ingest - Exception: Timeout reached while waiting on service!
ERROR tests/integration/kafka-connect/test_kafka_connect.py::test_kafka_connect_ingest_stateful - Exception: Timeout reached while waiting on service!
ERROR tests/integration/kafka-connect/test_kafka_connect.py::test_kafka_connect_bigquery_sink_ingest - Exception: Timeout reached while waiting on service!
==== 21 passed, 2008 deselected, 20 warnings, 5 errors in 712.74s (0:11:52) ====

Not sure if it's possible to restart this single job instead of the whole pipeline.

[sgomezvillamor]

No worries. Those kafka errors in testIntegrationBatch1 are being addressed in a separated PR.
I'm merging as well as closing the related issue. Thanks!