> On Feb. 19, 2021, 8:58 p.m., Madhan Neethiraj wrote: > > common/src/main/java/org/apache/atlas/repository/Constants.java > > Lines 212 (patched) > > <https://reviews.apache.org/r/73076/diff/17/?file=2246519#file2246519line212> > > > > - TASK_TYPE_PROPERTY_KEY and TASK_TYPE are duplicates > > - Is TASK_TYPE_NAME needed, given the type name will be stored in > > TASK_TYPE
TASK_TYPE_PROPERTY_KEY is the vertex type. Helps with retrieval (getAllTaskTypes). TASK_TYPE is the type of task e.g. PROPAGATION_ADD, PROPAGATION_DELETE, etc. > On Feb. 19, 2021, 8:58 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/tasks/TaskExecutor.java > > Lines 52 (patched) > > <https://reviews.apache.org/r/73076/diff/17/?file=2246532#file2246532line52> > > > > It looks like tasks are saved in the store only just before execution - > > in TaskConsumer.run(). That could be after a long time from this point. > > This increases the chances of loosing the tasks - consider Atlas > > shutdown/goes down/becomes passive. Consider saving all tasks to store > > before adding to taskList, to minimize the chances of loosing tasks. Tasks are committed as part of the parent transaction. They are passed on for execution once the transaction succeeds. If parent transactions fails, the task is removed. If it succeeds and server shuts down, they are retrieved upon server startup as part of pending transaction processing. > On Feb. 19, 2021, 8:58 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/tasks/TaskExecutor.java > > Lines 92 (patched) > > <https://reviews.apache.org/r/73076/diff/17/?file=2246532#file2246532line92> > > > > Task execution is skipped if task.getAdditionalInfo() is null; purpose > > of this condition is not clear. Consider adding a comment. I have updated the name of the method from getAdditionalInformation to getTaskVertex. > On Feb. 19, 2021, 8:58 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/tasks/TaskExecutor.java > > Lines 100 (patched) > > <https://reviews.apache.org/r/73076/diff/17/?file=2246532#file2246532line100> > > > > When will such tasks (that exceeded retry count) be removed from store? The tasks won't ever be removed. Only potential solution is to run purge operation. > On Feb. 19, 2021, 8:58 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/tasks/TaskExecutor.java > > Lines 123 (patched) > > <https://reviews.apache.org/r/73076/diff/17/?file=2246532#file2246532line123> > > > > - COMPLETE_WITH_ERRORS => FAILED? > > - when will failed tasks be removed from the store? In current logic, never. > On Feb. 19, 2021, 8:58 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java > > Lines 54 (patched) > > <https://reviews.apache.org/r/73076/diff/17/?file=2246535#file2246535line54> > > > > In case of HA deployment, startInternal() should be deferred until > > instanceIsActive() is called. Please refer to NotificationHookConsumer. I figured out the problem. I have addresed it. > On Feb. 19, 2021, 8:58 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java > > Lines 73 (patched) > > <https://reviews.apache.org/r/73076/diff/17/?file=2246535#file2246535line73> > > > > In case of HA deployment, instanceIsPassive() should stop processing > > queuedTasks, as another instance will start processing soon. In case of HA, passive instance will not process tasks. This is in line with other components. > On Feb. 19, 2021, 8:58 p.m., Madhan Neethiraj wrote: > > repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java > > Lines 122 (patched) > > <https://reviews.apache.org/r/73076/diff/17/?file=2246535#file2246535line122> > > > > Why only pending tasks? All tasks in the store are to be executed, > > right? That is right. All tasks that are available should be run. - Ashutosh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/73076/#review222619 ----------------------------------------------------------- On Feb. 19, 2021, 3:49 a.m., Ashutosh Mestry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/73076/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2021, 3:49 a.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. > > **REST APIs** > +-----------------+--------+----------+----------------------------------------------------------------------+ > | api/admin/tasks | GET | guids | (optional) List of guids to be > fetched. | > | | | | If list of guids supplied is empty > all tasks | > | | | | that are in PENDING state are > fetched. If list | > | | | | guids is specified, tasks > corresponding to that guid are fetched. | > +-------------------+------+----------+----------------------------------------------------------------------+ > |api/admin/task |DELETE| guids | List of guids to be deleted. No > action is taken if list of guids | > | | | | supplied is empty or a Task guid does > not exist. | > +-------------------+------+----------+----------------------------------------------------------------------+ > > **New Updates** > - Separate log file for tasks. > - TaskDef now has _startTime_ and _endTime_ to indicating start and end of > task. > - Refactored parameter passing during task creation. It now accepts Map > instead of Object array. Each task creates its own set of parameters. > - Reduced use of TASKS_ENABLE flag checks. > - Added _AtlasEntity.getPendingTasks()_ that indicidate presence of tasks for > a given entity. > > > Diffs > ----- > > common/src/main/java/org/apache/atlas/repository/Constants.java 61abfcaca > distro/src/conf/atlas-log4j.xml 7df963eb2 > intg/src/main/java/org/apache/atlas/AtlasConfiguration.java 08d6c9d4a > intg/src/main/java/org/apache/atlas/model/instance/AtlasEntity.java > 4d8c94894 > intg/src/main/java/org/apache/atlas/model/tasks/AtlasTask.java PRE-CREATION > intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java 27c7f7391 > intg/src/main/java/org/apache/atlas/type/Constants.java 7ee852086 > 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/graph/GraphBackedSearchIndexer.java > d0ffb853f > > 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/EntityGraphRetriever.java > 8208d11c2 > > 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/tasks/TaskRegistry.java > PRE-CREATION > repository/src/test/java/org/apache/atlas/TestModules.java 71b0a4a68 > > repository/src/test/java/org/apache/atlas/repository/tagpropagation/ClassificationPropagationTestWithTasks.java > PRE-CREATION > 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 7de3536f4 > > server-api/src/main/java/org/apache/atlas/listener/ActiveStateChangeHandler.java > bb7f8fcc4 > test-tools/pom.xml ccad452ea > test-tools/src/main/resources/log4j.properties 4db0598ad > webapp/src/main/java/org/apache/atlas/BeanUtil.java ef5a741ee > webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java > e8fc111a6 > webapp/src/main/java/org/apache/atlas/web/service/EmbeddedServer.java > d6658fe29 > webapp/src/test/java/org/apache/atlas/web/resources/AdminResourceTest.java > 77422b2a5 > > > Diff: https://reviews.apache.org/r/73076/diff/18/ > > > Testing > ------- > > **Unit tests** > New tests. > Additional unit tests covering end to end scenarios. > > **Manual tests** > - Via web ui. > - REST APIs. > > **Volume test** > - Adding classification with propagations for lineage with 2000 elements. > > **Pre-commit build** > https://ci-builds.apache.org/job/Atlas/job/PreCommit-ATLAS-Build-Test/281/ > > > Thanks, > > Ashutosh Mestry > >
