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

fix(ingest/mssql): remove lower() method from sql_common get_db_name #9919

Conversation

sleeperdeep
Copy link
Contributor

@sleeperdeep sleeperdeep commented Feb 26, 2024

Closes #9857

@github-actions github-actions bot added ingestion PR or Issue related to the ingestion of metadata community-contribution PR or Issue raised by member(s) of DataHub Community labels Feb 26, 2024
@RyanHolstien RyanHolstien requested a review from hsheth2 February 26, 2024 18:34
@hsheth2 hsheth2 requested review from mayurinehate and removed request for hsheth2 March 1, 2024 21:01
@hsheth2
Copy link
Collaborator

hsheth2 commented Mar 1, 2024

@sleeperdeep looks like this is causing some tests to fail

@sleeperdeep
Copy link
Contributor Author

@hsheth2 @mayurinehate
Hi guys! Any objections to merge these changes?

@mayurinehate
Copy link
Collaborator

@hsheth2 @mayurinehate Hi guys! Any objections to merge these changes?

My primary concern is that this is breaking change for URNs and this may impact existing setups. Please allow me some more time to assess the impact. @hsheth2 feel free to add if you have any thoughts on this.

@sleeperdeep
Copy link
Contributor Author

sleeperdeep commented May 9, 2024

Hi @hsheth2 @mayurinehate! What is your thoughts? Any additional things, that should be fixed inside this PR?

@sleeperdeep
Copy link
Contributor Author

@hsheth2 @mayurinehate @mhw @rubiojr Are you here?

@sleeperdeep
Copy link
Contributor Author

@mayurinehate ?

@mayurinehate
Copy link
Collaborator

mayurinehate commented Jun 20, 2024

@mayurinehate ?

@sleeperdeep We have identified that this would primarily affect container urns for a few (3-tier) sources + stored procedure/jobs urns for sql server. First one being relatively not so important for any other relationships than "container hierachy" and second one being relatively newer feature, this change seems fine and not so harmful. Re-running with stateful ingestion should automatically clear up the entities with old URNS and add entities with new URNs, therefore not duplicating the containers or jobs. Do you mind adding a note in updating-datahub about this change and how to mitigate this on similar lines as this -> https://github.com/datahub-project/datahub/pull/9601/files#diff-8a490856796965613edb9d4377cc46b5019bbd92d76d59096189e55bd22e709b

@mayurinehate
Copy link
Collaborator

@sleeperdeep the diff doesn't look correct. Can you please check and remove accidentally added files ?

@sleeperdeep
Copy link
Contributor Author

@mayurinehate Yes, I`m in progress with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community-contribution PR or Issue raised by member(s) of DataHub Community ingestion PR or Issue related to the ingestion of metadata pending-submitter-response Issue/request has been reviewed but requires a response from the submitter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

convert_urns_to_lowercase: inconsistency of usage in SQLAlchemySource.get_db & get_identifier methods
3 participants