Fix 14040: Part 1 Remove get_by_name calls from topology #14098

OnkarVO7 · 2023-11-24T07:03:26Z

Describe your changes:

Remove get_by_name calls from topology

Type of change:

Checklist:

I have read the CONTRIBUTING document.
My PR title is Fixes <issue-number>: <short explanation>
I have commented on my code, particularly in hard-to-understand areas.
For JSON Schema changes: I updated the migration scripts or explained why it is not needed.

github-actions · 2023-11-24T07:09:47Z

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

github-actions · 2023-11-24T10:32:08Z

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

github-actions · 2023-11-24T10:54:38Z

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

github-actions · 2023-11-24T11:33:55Z

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

github-actions · 2023-11-24T11:42:52Z

The Python checkstyle failed.

Please run make py_format and py_format_check in the root of your repository and commit the changes to this PR.
You can also use pre-commit to automate the Python code formatting.

You can install the pre-commit hooks with make install_test precommit_install.

OnkarVO7 · 2023-11-27T05:01:43Z

ingestion/src/metadata/ingestion/api/topology_runner.py

            for dependency in stage.consumer or []  # root nodes do not have consumers
        ]
        return fqn._build(  # pylint: disable=protected-access
            *context_names, entity_request.name.__root__
        )

-    def update_context(self, stage: NodeStage, entity: Entity):
+    def update_context(self, stage: NodeStage, entity_name: str):


We'll store the entity_name in the topology context instead of the entity_fqn and build the fqn on the fly wherever required. This is mainly because we need the entity_name in few places like here and here

sounds good! We can add a comment about this in the code itself

pmbrull · 2023-11-27T08:41:34Z

ingestion/src/metadata/ingestion/api/topology_runner.py


        # We have ack the sink waiting for a response, but got nothing back
-        if stage.must_return and entity is None:
+        if stage.must_return and entity_name is None:


does it make sense to have the must_return anymore? We are not running any GET so we might as well skip this validation I guess.

Another approach which I might like a bit better is to keep a must_return only for important pieces, such as the Services. Those we want to be able to GET all the time otherwise there's no point continuing the ingestion

Added a get_by_name call only if the must_return attribute is kept true

pmbrull · 2023-11-27T08:43:02Z

ingestion/src/metadata/ingestion/api/topology_runner.py

@@ -283,7 +264,7 @@ def _(
            )

        # We want to keep the full payload in the context
-        self.update_context(stage=stage, entity=right)
+        self.update_context(stage=stage, entity_name=right)


in this method we are missing removing the GET I believe

and same as with the lineage, not sure why we are passing the full right to the entity_name

Modified the call

pmbrull · 2023-11-27T08:43:46Z

ingestion/src/metadata/ingestion/api/topology_runner.py

@@ -245,7 +226,7 @@ def _(
        lineage has been properly drawn. We'll skip the process for now.
        """
        yield entity_request
-        self.update_context(stage=stage, entity=right)
+        self.update_context(stage=stage, entity_name=right)


Maybe we can update this call. Not sure why we are passing the full right to the entity_name

pmbrull · 2023-11-27T08:44:36Z

ingestion/src/metadata/ingestion/source/dashboard/dashboard_service.py

+                        service_name=self.context.dashboard_service,
+                        data_model_name=datamodel,
+                    )
+                    datamodel_entity = self.metadata.get_by_name(


why are we running a GET here?

pmbrull

We are still adding some GETs required for the lineage. We will address this in a followup PR caching OM's state

sonarqubecloud · 2023-11-27T10:45:24Z

[open-metadata-ingestion] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

42.3% Coverage
6.0% Duplication

* Changed for database * Added changes for dashboard_service * Changed for messaging service * Changed for mlmodel service * Changed for pipeline service * Changed for search service * Changed for objectstore service * fixed wrong import * fixed lint * fixes * fixed pytests * fixed domo db pytest * Fixed review comments

Changed for database

341bfa6

OnkarVO7 requested a review from a team as a code owner November 24, 2023 07:03

OnkarVO7 marked this pull request as draft November 24, 2023 07:03

github-actions bot added Ingestion safe to test Add this label to run secure Github workflows on PRs labels Nov 24, 2023

OnkarVO7 added 7 commits November 24, 2023 13:19

Added changes for dashboard_service

54fd728

Changed for messaging service

51dd010

Changed for mlmodel service

a7539d5

Changed for pipeline service

b6906ea

Changed for search service

ff051a5

Changed for objectstore service

ae553f2

Merge branch 'main' into rmv_get_by_name_topology

e6043f3

fixed wrong import

6623271

fixed lint

3ca1356

fixes

80374fd

OnkarVO7 added 2 commits November 24, 2023 19:33

fixed pytests

c1c8dc7

fixed domo db pytest

5abb32e

OnkarVO7 commented Nov 27, 2023

View reviewed changes

OnkarVO7 marked this pull request as ready for review November 27, 2023 05:02

OnkarVO7 mentioned this pull request Nov 27, 2023

Refactor PUT vs. PATCH in the Ingestion Framework #14040

Closed

6 tasks

pmbrull reviewed Nov 27, 2023

View reviewed changes

Fixed review comments

eda6ab8

OnkarVO7 requested a review from pmbrull November 27, 2023 10:32

pmbrull approved these changes Nov 27, 2023

View reviewed changes

OnkarVO7 merged commit ebb2317 into main Nov 27, 2023
12 checks passed

OnkarVO7 deleted the rmv_get_by_name_topology branch November 27, 2023 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 14040: Part 1 Remove get_by_name calls from topology #14098

Fix 14040: Part 1 Remove get_by_name calls from topology #14098

OnkarVO7 commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

OnkarVO7 Nov 27, 2023

pmbrull Nov 27, 2023

pmbrull Nov 27, 2023

OnkarVO7 Nov 27, 2023

pmbrull Nov 27, 2023

pmbrull Nov 27, 2023

OnkarVO7 Nov 27, 2023

pmbrull Nov 27, 2023

pmbrull Nov 27, 2023

pmbrull left a comment

sonarqubecloud bot commented Nov 27, 2023

Fix 14040: Part 1 Remove get_by_name calls from topology #14098

Fix 14040: Part 1 Remove get_by_name calls from topology #14098

Conversation

OnkarVO7 commented Nov 24, 2023

Describe your changes:

Type of change:

Checklist:

github-actions bot commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

github-actions bot commented Nov 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmbrull left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 27, 2023