----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/73076/#review222558 -----------------------------------------------------------
intg/src/main/java/org/apache/atlas/AtlasConfiguration.java Lines 79 (patched) <https://reviews.apache.org/r/73076/#comment311655> DEFERRED_ACTION_ENABLED("atlas.deferred.action.enabled", true) intg/src/main/java/org/apache/atlas/model/tasks/TaskDef.java Lines 45 (patched) <https://reviews.apache.org/r/73076/#comment311656> "model" package should only have data that is exchanged with Atlas clients - via REST API or notifications. Please move these constants out of model package, to org.apache.atlas.repository.Constants - along with other vertext property constants intg/src/main/java/org/apache/atlas/model/tasks/TaskDef.java Lines 63 (patched) <https://reviews.apache.org/r/73076/#comment311657> Consider using Map<String, String> as type for parameters. intg/src/main/java/org/apache/atlas/model/tasks/TaskDef.java Lines 64 (patched) <https://reviews.apache.org/r/73076/#comment311666> Consider tracking start and end time of task execution. intg/src/main/java/org/apache/atlas/model/tasks/TaskDef.java Lines 65 (patched) <https://reviews.apache.org/r/73076/#comment311667> Since TaskDef will be serialized/deserialized, consider using Map<String, String> as type for additionalInfo, instead of Object. intg/src/main/java/org/apache/atlas/utils/AtlasJson.java Lines 175 (patched) <https://reviews.apache.org/r/73076/#comment311658> the implementation doesn't seem to require/enforce that paramter is a linkedHashMap. If this is irrelevant, consider remaing this method to fromObject(): public static <T> T fromObject(Object obj, Class<T> type) { T ret = null; if (obj != null) { ret = mapper.convertValue(obj, type); if (ret instanceof Struct) { ((Struct) ret).normalize(); } } return ret; } Rename AtlasType.fromLinkedHashMap() as well, to AtlasType.fromObject(). repository/src/main/java/org/apache/atlas/GraphTransactionInterceptor.java Lines 241 (patched) <https://reviews.apache.org/r/73076/#comment311659> Consider removing "!AtlasConfiguration.TASKS_USE_ENABLED.getBoolean()" from here, as RequestContext.get().getQueuedTasks() must be empty is deferred-actions are disabled. It better to isolate this flag to fewer places. Also, in what condition would "BeanUtilRepo.getBean(TaskManagement.class)" be null? It might be better to initialize this once, instead of calling for each transaction. repository/src/main/java/org/apache/atlas/repository/store/graph/v2/AtlasEntityStoreV2.java Line 119 (original), 122 (patched) <https://reviews.apache.org/r/73076/#comment311660> I suggest this logic, of forking on AtlasConfiguration.TASKS_USE_ENABLED.getBoolean(), be buried inside EntityGraphRetriever - to minimize exposing TASKS_USE_ENABLED. repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Lines 1926 (patched) <https://reviews.apache.org/r/73076/#comment311669> List of entities to propagate must be computed while processing the entity creation/update; the result of this computation must be stored in the deferred-action and executed later. Computation of entities to propagate the classification must not be deferred. Please review and update. repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Line 2060 (original), 2108 (patched) <https://reviews.apache.org/r/73076/#comment311668> Move #2118 - #2121 to here: if (isPropagationEnabled(classificationVertex)) { if (AtlasConfiguration.TASKS_USE_ENABLED.getBoolean()) { RequestContext.get().addClassificationVertexId(classificationVertex.getId().toString()); entityVertices = new ArrayList<>(); } else { entityVertices = deleteDelegate.getHandler().removeTagPropagation(classificationVertex); if (LOG.isDebugEnabled()) { LOG.debug("Number of propagations to delete -> {}", entityVertices.size()); } } } else { entityVertices = new ArrayList<>(); } repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java Lines 92 (patched) <https://reviews.apache.org/r/73076/#comment311664> Move private methods after all public and protected methods. repository/src/main/java/org/apache/atlas/tasks/TaskManagement.java Lines 100 (patched) <https://reviews.apache.org/r/73076/#comment311665> Consider changing parameters to a Map, instead of an array. server-api/src/main/java/org/apache/atlas/RequestContext.java Lines 65 (patched) <https://reviews.apache.org/r/73076/#comment311663> its not clear what 'classificationVertexIds' mean in request context. If this is about deferred-action context, consider adding: class DeferredActionContext { List<Integer> classificationVertexIds; } ..and add DeferredActionContext in RequestContext: class RequestContext DeferredActionContext deferredActionContext = new DeferredActionContext(); Instead of String, consider storing vertex-id - which is an integer. server-api/src/main/java/org/apache/atlas/RequestContext.java Lines 81 (patched) <https://reviews.apache.org/r/73076/#comment311662> Consider avoiding duplication of AtlasConfiguration.TASKS_USE_ENABLED in Request context as RequestContext.skipPropagation. Remove data member skipPropagation, and have its callers directly check for AtlasConfiguration.TASKS_USE_ENABLED webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java Lines 160 (patched) <https://reviews.apache.org/r/73076/#comment311661> final - Madhan Neethiraj On Jan. 25, 2021, 11:08 p.m., Ashutosh Mestry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/73076/ > ----------------------------------------------------------- > > (Updated Jan. 25, 2021, 11:08 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 08d6c9d4a > 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 > ce58e9aa4 > > 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 7de3536f4 > > 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/12/ > > > 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 > >
