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

DG-1924 | Update task vertex w impacted-vertices counts for remaining types of propagation #3897

Merged
merged 4 commits into from
Jan 13, 2025

Conversation

abhijeet-atlan
Copy link

Change description

Description here

Type of change

  • Bug fix (fixes an issue)
  • New feature (adds functionality)

Related issues

Fix #1

Checklists

Development

  • Lint rules pass locally
  • Application changes have been tested thoroughly
  • Automated tests covering modified code pass

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached as necessary
  • "Ready for review" label attached and reviewers assigned
  • Changes have been reviewed by at least one other contributor
  • Pull request linked to task tracker where applicable

currentTask.setAssetsCountToPropagate((long) totalCount);

//update the 'assetsCountToPropagate' in the current task vertex.
AtlasVertex currentTaskVertex = (AtlasVertex) graph.query().has(TASK_GUID, currentTask.getGuid()).vertices().iterator().next();
Copy link
Collaborator

Choose a reason for hiding this comment

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

make this a method, instead of a single liner. As far as i remember, method already exists which can get taskVertex based on taskGuid

Copy link
Author

Choose a reason for hiding this comment

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

getting the taskVertex gets from the POJO rather than inside the graph. This graph query is to make sure the change is persisted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would still recommend creating a generic method in the mapper or helper file for this. This logic can be utilised in the future ofcourse. And yes in V2 ig you will be doing that.

graph.commit();

currentTask.setAssetsCountPropagated(currentTask.getAssetsCountPropagated() + totalCount);
currentTaskVertex.setProperty(TASK_ASSET_COUNT_PROPAGATED, currentTask.getAssetsCountPropagated());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is line 3233->3237 not a single graph commit? and instead 2?

Copy link
Author

Choose a reason for hiding this comment

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

the graph.commit(); is to commit the final count of all the tasks in the task vertex in the graph.

the next setter calls is to update the finished task count for the assetsCountPropagated

This specific method is responsible for the cleanup task propagation where the planner and executer are part of the same while loop, thats why the variables are updated after the task is finished rather than in real time.

The change required here is to split the planning and execution which will be done in the last versions of tag propagation.

for ref: check this and onward responses

currentTask.setAssetsCountToPropagate((long) impactedVertices.size() - 1);

//update the 'assetsCountToPropagate' in the current task vertex.
AtlasVertex currentTaskVertex = (AtlasVertex) graph.query().has(TASK_GUID, currentTask.getGuid()).vertices().iterator().next();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this one liner to a method call. As iterated previously in one of the comments

Copy link
Author

Choose a reason for hiding this comment

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

for all the comments asking to make the graph call into a method instead of a one liner, me and janaki has discussed this and I will be doing that in version 2. Thats part of the code cleanup.

currentTask.setAssetsCountToPropagate((long) verticesToRemove.size() + verticesToAddClassification.size() - 1);

//update the 'assetsCountToPropagate' in the current task vertex.
AtlasVertex currentTaskVertex = (AtlasVertex) graph.query().has(TASK_GUID, currentTask.getGuid()).vertices().iterator().next();
Copy link
Collaborator

Choose a reason for hiding this comment

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

here as well

Copy link
Author

Choose a reason for hiding this comment

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

for all the comments asking to make the graph call into a method instead of a one liner, me and janaki has discussed this and I will be doing that in version 2. Thats part of the code cleanup.

@abhijeet-atlan abhijeet-atlan merged commit dc614df into taskdg1924 Jan 13, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants