----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/73076/#review222660 -----------------------------------------------------------
intg/src/main/java/org/apache/atlas/model/instance/AtlasEntity.java Lines 99 (patched) <https://reviews.apache.org/r/73076/#comment311828> It will help to add following comment for pendingTasks field: private Set<String> pendingTasks; // read-only field i.e. value provided is ignored during entity create/update intg/src/main/java/org/apache/atlas/model/instance/AtlasEntity.java Lines 544 (patched) <https://reviews.apache.org/r/73076/#comment311829> equals() and hashCode() need not take pendingTasks value into account. intg/src/main/java/org/apache/atlas/model/tasks/AtlasTask.java Lines 184 (patched) <https://reviews.apache.org/r/73076/#comment311830> - getTaskVertex() => getAdditionalInfo()? - @JsonIgnore on additionalInfo: is it not necessary to serialize/deserialize additionalInfo? repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Line 145 (original), 164 (patched) <https://reviews.apache.org/r/73076/#comment311831> Consider injecting taskManagement in the constructor. And mark it as final. Its value shouldn't depend on DEFERRED_ACTION_ENABLED. And update setTasksUseFlag() from: setTasksUseFlag(TaskManagement taskManagement) to: setTasksUseFlag(boolean useTasks) repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Lines 2053 (patched) <https://reviews.apache.org/r/73076/#comment311834> impacted-vertices list depends on classification being propagated - right? Some edges might explicitly not propagate a classification. Please review how this is handled? Perhaps replace: entityRetriever.getImpactedVerticesV2(entityVertex) with: entityRetriever.getImpactedVerticesV2(entityVertex, null, classificationVertexId); repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Lines 2800 (patched) <https://reviews.apache.org/r/73076/#comment311835> Is this necessary here? Locks should be obtained much earlier in the add/update/remove classification sequence. Please review if attempting to obtain lock here can cause dead lock if 2 threads are holding lock on one entity each, and waiting to get lock on other entity. repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Lines 2811 (patched) <https://reviews.apache.org/r/73076/#comment311836> Instead of re-adding exising pendingTasks, consider adding only taskGuid - the new task. AtlasGraphUtilsV2.addEncodedProperty(entityVertex, PENDING_TASKS_PROPERTY_KEY, taskGuid); repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Lines 2814 (patched) <https://reviews.apache.org/r/73076/#comment311837> The locks is supposed to be released after commit is performed; here it is released before commit. How does this lock help? Please review similar usage in removePendingTaskFromEntityVertex() as well. repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Lines 2838 (patched) <https://reviews.apache.org/r/73076/#comment311832> Why would taskManagement be null, if it is injected during construction? repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphRetriever.java Lines 1668 (patched) <https://reviews.apache.org/r/73076/#comment311843> ret might already be a set. In that case, creation of HasSet<> can be avoided. repository/src/main/java/org/apache/atlas/repository/store/graph/v2/tasks/ClassificationTask.java Lines 90 (patched) <https://reviews.apache.org/r/73076/#comment311842> Should this be rollback() in case of failures i.e. block at #85? repository/src/main/java/org/apache/atlas/tasks/TaskExecutor.java Lines 42 (patched) <https://reviews.apache.org/r/73076/#comment311840> consumerThread is initialized only during construction - though in a private method called from constructor. Consider marking consumerThread as final, by moving the assignment in the private method to constructor. repository/src/main/java/org/apache/atlas/tasks/TaskExecutor.java Lines 152 (patched) <https://reviews.apache.org/r/73076/#comment311841> Is this commit() only for changes to task status? Or does this include entity/classfication updates too? In its later, then wouldn't graph DB status be incorrect in case of Exception i.e. block at #137? repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java Lines 57 (patched) <https://reviews.apache.org/r/73076/#comment311838> TaskRegistry must be injected for annotations on its methods to work - like GraphTransaction. Please review and update. repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java Lines 161 (patched) <https://reviews.apache.org/r/73076/#comment311839> print() might be done only when tasks on submitted. Consider moving this call to where a task execution completes. - Madhan Neethiraj On March 8, 2021, 6:19 p.m., Ashutosh Mestry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/73076/ > ----------------------------------------------------------- > > (Updated March 8, 2021, 6:19 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. > > **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 771287f75 > 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/AtlasTypeRegistry.java 4c7f8c681 > 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/discovery/AtlasDiscoveryServiceTest.java > 9846d4310 > > 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 37d23c23b > > 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 > test-tools/src/main/resources/solr/core-template/solrconfig.xml 4d7e444bf > 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/21/ > > > 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 > >
