----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/73076/#review222428 -----------------------------------------------------------
repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Lines 2037 (patched) <https://reviews.apache.org/r/73076/#comment311486> consider adding tags to impacted vertices in batches (25 or 50) and committing the transaction, batch commits will save time rather than committing a single transaction with large changes. repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Lines 2324 (patched) <https://reviews.apache.org/r/73076/#comment311478> include this if condition along with existing if condition in previous line(2323). This will avoid additional nested indentation repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapperWithTasks.java Lines 191 (patched) <https://reviews.apache.org/r/73076/#comment311485> In line 191 and 203, we pass classificationNames/classifications in addition to classifications vertexIds. Since classification vertexIds is available, we may not need to pass classificationNames or Classifications. These can be mapped from classificationVertex using entityGraphRetriever. repository/src/main/java/org/apache/atlas/repository/store/graph/v2/tasks/ClassificationPropagationTasks.java Lines 20 (patched) <https://reviews.apache.org/r/73076/#comment311479> nit: remove unused import repository/src/main/java/org/apache/atlas/repository/store/graph/v2/tasks/ClassificationPropagationTasks.java Lines 83 (patched) <https://reviews.apache.org/r/73076/#comment311484> this line always passes 0th classificationVertexId instead of i'th: classificationVertexIds.get(0) => classificationVertexIds.get(i) repository/src/main/java/org/apache/atlas/tasks/TaskExecutor.java Lines 103 (patched) <https://reviews.apache.org/r/73076/#comment311482> taskDef status here is always set to 'COMPLETE' irrespective of the outcome of task.run() task.run() should return the status of the execution and it should be set to taskDef. Please review. repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java Lines 55 (patched) <https://reviews.apache.org/r/73076/#comment311480> taskTypeTaskFactoryMap => taskTypeFactoryMap repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java Lines 263 (patched) <https://reviews.apache.org/r/73076/#comment311481> consider soft deleting the task vertex. It will be useful to check the status of the completed or failed tasks. We can purge the task vertices periodically (every hour, every day) or if some threshold is reached to avoid proliferations of these vertices. repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java Lines 267 (patched) <https://reviews.apache.org/r/73076/#comment311483> why update status of taskVertex and hard delete it? repository/src/test/java/org/apache/atlas/repository/tagpropagation/ClassificationPropagationTest.java Lines 369 (patched) <https://reviews.apache.org/r/73076/#comment311477> nit: refactor sleep/pause to a separate method. webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java Lines 750 (patched) <https://reviews.apache.org/r/73076/#comment311476> deleting a task should not be exposed through REST API (even as admin). tasks which needs to be deleted should be handled probably through a utility (similar to repair index) - Sarath Subramanian On Dec. 16, 2020, 3:42 p.m., Ashutosh Mestry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/73076/ > ----------------------------------------------------------- > > (Updated Dec. 16, 2020, 3:42 p.m.) > > > Review request for atlas, Jayendra Parab, Madhan Neethiraj, Nikhil Bonte, > Nixon Rodrigues, and Sarath Subramanian. > > > Bugs: ATLAS-3919 > https://issues.apache.org/jira/browse/ATLAS-3919 > > > Repository: atlas > > > Description > ------- > > **Approach** > Model: _TaskDef_ > - Definition of task. > - Contains status of the task for querying. > > _AbstractTask_ > - Takes TaskDef as input and calls run. > - The concrete tasks has logic for doing real work. > > _TaskFactory_ > > - Creates tasks given task type. > - Enumerates all the tasks that are supported. > > _TaskManagement_ > > - Component/Service. Constructed after all existing components are > constructed. > - Provides a simple interface for adding tasks. > - Processes tasks that are in pending state. > - Supports HA mode, where the tasks are not executed on passive instance. > > _TaskManagement.Registry_ > > - CRUD operations with TaskDef vertex. > > _TaskManagement.Processor_ > - Provides mechanism for provides single queue for processing tasks in a way > that is non-blocking. > - Uses task type and the associated TaskFactory to construct concrete task. > > Changes to _EntityGraphMapper_ > There are 3 new methods added that use portion of existing classification > propagation logic and create a separate method. > > New class _EntityGraphMapperWithTasks_ acts as a proxy for EntityGraphMapper. > All calls that operate without tasks get channeled to the existing > implementation. The ones involving tasks are modified to use the new task > framework. > > **Concurrent Changes to Classifications** > Via REST APIs and operations from web UI, it is possible to make changes to > the same entity. This situation will be more pronounced in the new setup. > > The existing framework has > _GraphTransactionInterceptor.lockObjectAndReleasePostCommit_. This provides > ReentrantLock on an entity that is identified by the supplied GUID. This will > cause a lock on the entity and all the impacted vertices. This has potential > for adversely impacting scalability. > > > Diffs > ----- > > intg/src/main/java/org/apache/atlas/AtlasConfiguration.java ea9f26d47 > intg/src/main/java/org/apache/atlas/model/tasks/TaskDef.java PRE-CREATION > intg/src/main/java/org/apache/atlas/type/AtlasType.java b30b483e0 > intg/src/main/java/org/apache/atlas/utils/AtlasJson.java abeddf640 > intg/src/test/resources/atlas-application.properties 7e74d5107 > repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java > 86b369fc9 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2.java > 244072289 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java > 2cfcc0bc1 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapperWithTasks.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/store/graph/v2/tasks/ClassificationPropagateTaskFactory.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/store/graph/v2/tasks/ClassificationPropagationTasks.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/store/graph/v2/tasks/ClassificationTask.java > PRE-CREATION > repository/src/main/java/org/apache/atlas/tasks/AbstractTask.java > PRE-CREATION > repository/src/main/java/org/apache/atlas/tasks/TaskExecutor.java > PRE-CREATION > repository/src/main/java/org/apache/atlas/tasks/TaskFactory.java > PRE-CREATION > repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java > PRE-CREATION > repository/src/main/java/org/apache/atlas/util/BeanUtilRepo.java > PRE-CREATION > > repository/src/test/java/org/apache/atlas/repository/tagpropagation/ClassificationPropagationTest.java > 84e9bfa04 > repository/src/test/java/org/apache/atlas/tasks/TaskExecutorTest.java > PRE-CREATION > repository/src/test/java/org/apache/atlas/tasks/TaskManagementTest.java > PRE-CREATION > repository/src/test/java/org/apache/atlas/tasks/TaskRegistryTest.java > PRE-CREATION > server-api/src/main/java/org/apache/atlas/RequestContext.java 216ba08d3 > > server-api/src/main/java/org/apache/atlas/listener/ActiveStateChangeHandler.java > bb7f8fcc4 > webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java > b20b40474 > webapp/src/test/java/org/apache/atlas/web/resources/AdminResourceTest.java > 77422b2a5 > > > Diff: https://reviews.apache.org/r/73076/diff/8/ > > > Testing > ------- > > **Unit tests** > New tests. > > **Manual tests** > In-progress. > > **Volume test** > Pending. > > **Pre-commit build** > https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/281/ > > > Thanks, > > Ashutosh Mestry > >
