DG-1924 | Add 'assetsCountToPropagate' and 'assetsCountPropagated' in task vertex. #4032

abhijeet-atlan · 2025-01-27T07:13:44Z

DG1924
Design plan ref :Execution Plan - Phase v1 - WIP | 1. Task Vertex
Test cases: DG1924 - TestCase

The task for this ticket:

Add 'assetsCountToPropagate' and 'assetsCountPropagated' params to the task vertex and

update 'assetsCountToPropagate' with a count of the assets count to propagate once the planning phase is finished.
update 'assetsCountPropagated' with a count as the task progresses.

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

...itory/src/main/java/org/apache/atlas/repository/store/graph/v2/tasks/ClassificationTask.java

repository/src/main/java/org/apache/atlas/tasks/TaskFactory.java

jnkrmg

LGTM!

sumandas0 · 2025-01-28T06:25:28Z

.github/workflows/maven.yml

@@ -28,6 +28,8 @@ on:
      - master
      - lineageondemand
      - makerlogic
+      - taskdg1924deleteprop


Remove these

intg/src/main/java/org/apache/atlas/model/patches/AtlasPatch.java

repository/src/main/java/org/apache/atlas/repository/patches/AtlasPatchRegistry.java

repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java

repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java

repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java

sumandas0 · 2025-01-30T06:07:18Z

repository/src/main/java/org/apache/atlas/tasks/TaskRegistry.java

        RequestContext.get().setCurrentTask(task);

        task.setStartTime(new Date());
-
+        task.setAssetsCountToPropagate(runnableTask.getAssetsCountToPropagate());


Why we need to pass runnable task here?

As the task goes into progress the existing code updates the startTime and a few other details about the task. In the same scenario I needed to update value for assetsCountToPropagate with the number of tasks and for that I need the runnable.

Are you sure you have the real number here to set assetsCountToPropagate?
IMO here you will always get it as 0, as per previous code I saw while creating Task vertex we already write value as 0, I feel this is not needed

sumandas0 · 2025-01-30T06:09:58Z

repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java

+        //update the 'assetsCountToPropagate' in the current task vertex.
+        AtlasVertex currentTaskVertex = (AtlasVertex) graph.query().has(TASK_GUID, currentTask.getGuid()).vertices().iterator().next();
+        currentTaskVertex.setProperty(TASK_ASSET_COUNT_TO_PROPAGATE, currentTask.getAssetsCountToPropagate());
+        graph.commit();


don't use graph.commit()

We don't need to commit here given we will be committing to the graph just after 2 line

Wrote a new function updateTaskVertexProperty that is used everywhere and it doesn't have graph.commit() in it.

sumandas0 · 2025-01-30T06:11:34Z

repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java

+            //update the 'assetsCountToPropagate' in the current task vertex.
+            AtlasVertex currentTaskVertex = (AtlasVertex) graph.query().has(TASK_GUID, currentTask.getGuid()).vertices().iterator().next();
+            currentTaskVertex.setProperty(TASK_ASSET_COUNT_TO_PROPAGATE, currentTask.getAssetsCountToPropagate());
+            graph.commit();


remove graph.commit()

Wrote a new function updateTaskVertexProperty that is used everywhere and it doesn't have graph.commit() in it.

sumandas0 · 2025-01-30T06:12:27Z

repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java

@@ -3460,9 +3481,16 @@ public List<String> propagateClassification(String entityGuid, String classifica
            Boolean toExclude = propagationMode == CLASSIFICATION_PROPAGATION_MODE_RESTRICT_LINEAGE ? true:false;
            List<AtlasVertex> impactedVertices = entityRetriever.getIncludedImpactedVerticesV2(entityVertex, relationshipGuid, classificationVertexId, edgeLabelsToCheck,toExclude);

+            // update the 'assetsCountToPropagate' on in memory java object.
+            AtlasTask currentTask = RequestContext.get().getCurrentTask();
+            currentTask.setAssetsCountToPropagate((long) impactedVertices.size() - 1);


Create a method to update the task vertex

Wrote a new function updateTaskVertexProperty that is used everywhere and it doesn't have graph.commit() in it.

sumandas0 · 2025-01-30T06:24:31Z

repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java

+        //update the 'assetsCountToPropagate' in the current task vertex.
+        AtlasVertex currentTaskVertex = (AtlasVertex) graph.query().has(TASK_GUID, currentTask.getGuid()).vertices().iterator().next();
+        currentTaskVertex.setProperty(TASK_ASSET_COUNT_TO_PROPAGATE, currentTask.getAssetsCountToPropagate());
+        graph.commit();


remove graph.commit

Wrote a new function updateTaskVertexProperty that is used everywhere and it doesn't have graph.commit() in it.

sumandas0 · 2025-01-30T06:25:04Z

repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java

@@ -4351,6 +4416,15 @@ List<String> processClassificationEdgeDeletionInChunk(AtlasClassification classi
        int toIndex;
        int offset = 0;

+        // update the 'assetsCountToPropagate' on in memory java object.
+        AtlasTask currentTask = RequestContext.get().getCurrentTask();


too much code repetition, create a generic method for updating it

Wrote a new function updateTaskVertexProperty that is used everywhere and it doesn't have graph.commit() in it.

sumandas0 · 2025-01-30T06:26:12Z

...itory/src/main/java/org/apache/atlas/repository/store/graph/v2/tasks/ClassificationTask.java

@@ -100,9 +101,7 @@ public AtlasTask.Status perform() throws AtlasBaseException {

        try {
            setStatus(IN_PROGRESS);


nit: no need for this changes

sumandas0 · 2025-01-30T06:29:13Z

repository/src/main/java/org/apache/atlas/tasks/TaskRegistry.java

@@ -361,8 +363,8 @@ public List<AtlasTask> getTasksForReQueueGraphQuery() {
                .has(Constants.TASK_TYPE_PROPERTY_KEY, Constants.TASK_TYPE_NAME);

        List<AtlasGraphQuery> orConditions = new LinkedList<>();
-        orConditions.add(query.createChildQuery().has(Constants.TASK_STATUS, AtlasTask.Status.IN_PROGRESS));
-        orConditions.add(query.createChildQuery().has(Constants.TASK_STATUS, AtlasTask.Status.PENDING));
+        orConditions.add(query.createChildQuery().has(TASK_STATUS, AtlasTask.Status.IN_PROGRESS));


reason for these changes?

I have reverted these back

sumandas0 · 2025-01-30T06:30:09Z

.github/workflows/maven.yml

@@ -28,6 +28,7 @@ on:
      - master
      - lineageondemand
      - makerlogic
+      - tagpropv1master


remove this

will do once all review is complete and we are ready to move to master

sumandas0

Mostly reviews are for code quality

abhijeet-atlan · 2025-02-03T04:13:59Z

removed madu from review because of new team structure

…k vertex. The former will be updated with the total count of propagations to be done once the planning phase is complete and the task begins execution. The latter will be updated as the task progresses, reflecting the count of completed propagations at any given point.

…'refresh prop' and 'cleanup prop'

sumandas0 · 2025-02-04T22:22:45Z

repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java

@@ -55,6 +61,21 @@ public class TaskManagement implements Service, ActiveStateChangeHandler {
    private final RedisService redisService;
    private Thread watcherThread = null;

+    public void updateTaskVertexProperty(String propertyKey, AtlasGraph graph, long value, boolean isIncremental, BiConsumer<AtlasTask, Long> taskSetter) {


Looks fine, just add few descriptions how we are using BiConsumer, as its bit complicated to understand for reviewer

sumandas0

Just add proper description, all looks good now otherwise. Will approve

intg/src/main/java/org/apache/atlas/model/patches/AtlasPatch.java

repository/src/main/java/org/apache/atlas/repository/patches/AtlasPatchRegistry.java

repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java

repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java

nikhilbonte21 · 2025-02-05T06:20:39Z

repository/src/main/java/org/apache/atlas/tasks/TaskRegistry.java

        RequestContext.get().setCurrentTask(task);

        task.setStartTime(new Date());
-
+        task.setAssetsCountToPropagate(runnableTask.getAssetsCountToPropagate());


Are you sure you have the real number here to set assetsCountToPropagate?
IMO here you will always get it as 0, as per previous code I saw while creating Task vertex we already write value as 0, I feel this is not needed

repository/src/main/java/org/apache/atlas/tasks/TaskRegistry.java

server-api/src/main/java/org/apache/atlas/RequestContext.java

abhijeet-atlan requested review from Madusudanan, sumandas0, nikhilbonte21, jnkrmg and hr2904 January 27, 2025 07:13

abhijeet-atlan force-pushed the tagpropv1master branch 3 times, most recently from bc0322a to cd6f2a3 Compare January 27, 2025 07:40

jnkrmg requested changes Jan 28, 2025

View reviewed changes

...itory/src/main/java/org/apache/atlas/repository/store/graph/v2/tasks/ClassificationTask.java Show resolved Hide resolved

repository/src/main/java/org/apache/atlas/tasks/TaskFactory.java Outdated Show resolved Hide resolved

abhijeet-atlan force-pushed the tagpropv1master branch from e3485d8 to a118278 Compare January 28, 2025 05:33

abhijeet-atlan requested a review from jnkrmg January 28, 2025 05:33

jnkrmg previously approved these changes Jan 28, 2025

View reviewed changes

sumandas0 reviewed Jan 28, 2025

View reviewed changes

abhijeet-atlan dismissed jnkrmg’s stale review via 395213f January 28, 2025 12:44

abhijeet-atlan force-pushed the tagpropv1master branch 2 times, most recently from 2ea2057 to 7ff6994 Compare January 28, 2025 13:18

abhijeet-atlan requested review from sumandas0 and jnkrmg January 28, 2025 13:19

abhijeet-atlan force-pushed the tagpropv1master branch from ed9f39a to 398085d Compare January 28, 2025 13:23

hr2904 requested changes Jan 28, 2025

View reviewed changes

abhijeet-atlan requested a review from hr2904 January 29, 2025 11:53

abhijeet-atlan added the DO NOT MERGE label Jan 29, 2025

sumandas0 reviewed Jan 30, 2025

View reviewed changes

sumandas0 requested changes Jan 30, 2025

View reviewed changes

abhijeet-atlan requested review from sumandas0 and removed request for Madusudanan February 3, 2025 03:45

jnkrmg and others added 3 commits February 4, 2025 11:58

review comment fixes

65d920d

fixing dry for task vertex updation

61d58dd

abhijeet-atlan added 7 commits February 4, 2025 11:58

fixing dry for task vertex updation

dfddb79

removed redundant code for 'add prop', 'text update', 'delete prop', …

b4ce7af

…'refresh prop' and 'cleanup prop'

reverted taskregistry

ed847c1

removed all repeating code for 6 prop types

c257421

more fixes based on review comments

adc6ed1

refactored 'updateTaskVertexProperty'

fdca703

updated maven.yaml

164ec29

abhijeet-atlan force-pushed the tagpropv1master branch from dbae477 to 164ec29 Compare February 4, 2025 06:29

sumandas0 reviewed Feb 4, 2025

View reviewed changes

jnkrmg previously approved these changes Feb 5, 2025

View reviewed changes

added docstring to explain functionality to 'updateTaskVertexProperty'

4e8c6df

abhijeet-atlan dismissed jnkrmg’s stale review via 4e8c6df February 5, 2025 04:04

nikhilbonte21 requested changes Feb 5, 2025

View reviewed changes

intg/src/main/java/org/apache/atlas/model/patches/AtlasPatch.java Outdated Show resolved Hide resolved

repository/src/main/java/org/apache/atlas/repository/patches/AtlasPatchRegistry.java Outdated Show resolved Hide resolved

nikhilbonte21 requested changes Feb 5, 2025

View reviewed changes

repository/src/main/java/org/apache/atlas/repository/store/graph/v1/DeleteHandlerV1.java Outdated Show resolved Hide resolved

nikhilbonte21 requested changes Feb 5, 2025

View reviewed changes

repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java Outdated Show resolved Hide resolved

nikhilbonte21 requested changes Feb 5, 2025

View reviewed changes

abhijeet-atlan added 4 commits February 5, 2025 13:27

changed signature and usage for 'updateTaskVertexProperty'

14db31d

deleted not used function 'updateTaskVertexProperty'

102ae40

experiment to check if everything works

64775b4

removed commented out code

42540a0

abhijeet-atlan requested review from sumandas0 and nikhilbonte21 February 5, 2025 10:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DG-1924 | Add 'assetsCountToPropagate' and 'assetsCountPropagated' in task vertex. #4032

DG-1924 | Add 'assetsCountToPropagate' and 'assetsCountPropagated' in task vertex. #4032

abhijeet-atlan commented Jan 27, 2025 •

edited

Loading

jnkrmg left a comment

sumandas0 Jan 28, 2025

abhijeet-atlan Jan 28, 2025

sumandas0 Jan 30, 2025

abhijeet-atlan Feb 2, 2025

nikhilbonte21 Feb 5, 2025

sumandas0 Jan 30, 2025

sumandas0 Jan 30, 2025

abhijeet-atlan Feb 2, 2025

sumandas0 Jan 30, 2025

abhijeet-atlan Feb 2, 2025

sumandas0 Jan 30, 2025

abhijeet-atlan Feb 2, 2025

sumandas0 Jan 30, 2025

abhijeet-atlan Feb 2, 2025

sumandas0 Jan 30, 2025

abhijeet-atlan Feb 2, 2025

sumandas0 Jan 30, 2025

sumandas0 Jan 30, 2025

abhijeet-atlan Feb 2, 2025

sumandas0 Jan 30, 2025

abhijeet-atlan Feb 2, 2025

sumandas0 left a comment

abhijeet-atlan commented Feb 3, 2025

sumandas0 Feb 4, 2025

abhijeet-atlan Feb 5, 2025

sumandas0 left a comment

nikhilbonte21 Feb 5, 2025

		@@ -100,9 +101,7 @@ public AtlasTask.Status perform() throws AtlasBaseException {

		try {
		setStatus(IN_PROGRESS);

DG-1924 | Add 'assetsCountToPropagate' and 'assetsCountPropagated' in task vertex. #4032

Are you sure you want to change the base?

DG-1924 | Add 'assetsCountToPropagate' and 'assetsCountPropagated' in task vertex. #4032

Conversation

abhijeet-atlan commented Jan 27, 2025 • edited Loading

Type of change

Related issues

Checklists

Development

Security

Code review

jnkrmg left a 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

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

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

sumandas0 left a comment

Choose a reason for hiding this comment

abhijeet-atlan commented Feb 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumandas0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhijeet-atlan commented Jan 27, 2025 •

edited

Loading