atlas-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Madhan Neethiraj <mad...@apache.org>
Subject Re: Review Request 51939: Framework to apply updates to types in the type-system
Date Wed, 21 Sep 2016 20:55:51 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51939/#review149854
-----------------------------------------------------------




repository/src/main/java/org/apache/atlas/services/AtlasTypeAttributePatch.java (line 61)
<https://reviews.apache.org/r/51939/#comment217631>

    "attributeDefinitions" is an array in structType and classType. Lets keep this consistent
here. Please refer to ./addons/hive-bridge/target/models/hive_model.json.



repository/src/main/java/org/apache/atlas/services/AtlasTypeAttributePatch.java (line 87)
<https://reviews.apache.org/r/51939/#comment217583>

    Consider adding "currentVersion == null ||".



repository/src/main/java/org/apache/atlas/services/AtlasTypeAttributePatch.java (line 103)
<https://reviews.apache.org/r/51939/#comment217579>

    Consider returning PatchResult() with the error message from exception blocks.
    
    It will be a good practice to minimize the number of returns from a method. I generally
define a variable named "ret" at the entry of the method; set it to appropriate value in the
body; and return from the end of the method. Make it easier to read and add debug tracing.



repository/src/main/java/org/apache/atlas/services/AtlasTypeAttributePatch.java (line 122)
<https://reviews.apache.org/r/51939/#comment217636>

    Since this loop iterates for each attribute  in the patch, should this call only check
for presense of the current patch attribute?



repository/src/main/java/org/apache/atlas/services/AtlasTypeAttributePatch.java (line 125)
<https://reviews.apache.org/r/51939/#comment217637>

    Consider rewritting this 'if' as below, for better readability:
    
    if(! typeContainsPatchAttrs) {
      if(action == PatchAction.ADD_ATTRIBUTES) {
        // add the atttribute
      } else {
        LOG.warn(action + ": invalid action for non-existing attribute. Ignored.");
      }
    } else {
      if(action == PatchAction.UPDATE_ATTRIBUTES) {
        // update the atttribute
      } else if(action == PatchAction.DELETE_ATTRIBUTES) {
        // delete the atttribute
      } else {
        LOG.warn(action + ": invalid action for already existing attribute. Ignored.");
      }
    }



repository/src/main/java/org/apache/atlas/services/AtlasTypeAttributePatch.java (line 133)
<https://reviews.apache.org/r/51939/#comment217641>

    "to the type" ==> "in type"



repository/src/main/java/org/apache/atlas/services/AtlasTypeAttributePatch.java (line 137)
<https://reviews.apache.org/r/51939/#comment217642>

    "to the type" ==> "from type"



repository/src/main/java/org/apache/atlas/services/AtlasTypeAttributePatch.java (line 162)
<https://reviews.apache.org/r/51939/#comment217643>

    Consider updating the error messagea to include more details (like typename): 
    
    "error in getting type '" + patchData.getTypeName() + "' from typesystem"
    
    Please review other error messages as well.



repository/src/main/java/org/apache/atlas/services/AtlasTypeAttributePatch.java (line 186)
<https://reviews.apache.org/r/51939/#comment217646>

    Is it necessary to convert to JSON? Wouldn't iterating on currentAttributes work?
    
    if(currentAttributes != null) {
      for(AttributeDefintion attributeDef : currentAttributes) {
        s.add(attributeDef.name);
      }
    }



repository/src/main/java/org/apache/atlas/services/AtlasTypePatch.java (line 77)
<https://reviews.apache.org/r/51939/#comment217653>

    Consider adding attributeDefinitions as a first class attribute - to save from having
to tranform from/to JSON in many places. When we add support for other type of patches (like
types..), appropraite members can be added here.
    
    class PatchData {
      private String                   action;
      private String                   typeName;
      ..
      private List<AttributeDefintion> attributeDefinitions;
      private Map<String, Object>      params;
    }



repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java (line 171)
<https://reviews.apache.org/r/51939/#comment217654>

    checkForPatches() call will be missed in HA deployments. Make sure to call from instanceIsActive()
- just as restoreTypeSystem() is called.



repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java (line 190)
<https://reviews.apache.org/r/51939/#comment217658>

    patchFile can be null. Please update to handle this condition.



repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java (line 220)
<https://reviews.apache.org/r/51939/#comment217659>

    Supported actions: ADD_ATTRIBUTE, UPDATE_ATTRIBUTE, DELETE_ATTRIBUTE



repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java (line 225)
<https://reviews.apache.org/r/51939/#comment217660>

    result could be null. Please update to handle this condition.



repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java (line 228)
<https://reviews.apache.org/r/51939/#comment217661>

    e.printStackTrace() ==> LOG.error(..., e);


- Madhan Neethiraj


On Sept. 21, 2016, 7:51 a.m., Sarath Kumar Subramanian wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51939/
> -----------------------------------------------------------
> 
> (Updated Sept. 21, 2016, 7:51 a.m.)
> 
> 
> Review request for atlas, Madhan Neethiraj, Shwetha GS, and Suma Shivaprasad.
> 
> 
> Bugs: ATLAS-1174
>     https://issues.apache.org/jira/browse/ATLAS-1174
> 
> 
> Repository: atlas
> 
> 
> Description
> -------
> 
> 1. Introduce "version" attribute to all types in the type-system, this helps to track
changes made to the default types (hive, sqoop, falcon and storm types) and user created types.
If version is not mentioned during creation of a type, default version "1.0" is assigned (optional
attribute).
> 2. Using the version attributed for types, introduce a patch framework for type system.
This framework applies patches to a type using its version number and can be used during upgrade
- add new attributes to an existing types and it will be run during atlas startup.
> The sequence of steps:
> a. During atlas startup, check $ATLAS_HOME/models/patches directory for any available
patch files (json files). If there any patch files handle them.
> b. Sample patch json file looks like:
> {
> "patches": [
> { 
> "action": "ADD_ATTRIBUTE",
> "typeName": "hive_column",
> "applyToVersion": "1.0",
> "updateToVersion": "2.0",
> "actionParams": [
> { "name": "position", "dataTypeName": "int", "multiplicity": "optional", "isComposite":
false, "isUnique": false, "isIndexable": false, "reverseAttributeName": null }
> ]
> } ]
> }
> c. The framework updates the type in "typeName" for the matching version number and after
applying the patch, update the version to the one mentioned in "updateToVersion"
> d. The json file can have more than one action (array of actions).
> e. There can be multiple patch json files in the directory and are applied in the sort
order of the filename. eg:
> 001-hive_column_add_position.json
> 002-hive_column_add_anotherattribute.json
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/atlas/AtlasConstants.java 17ffbd7 
>   common/src/main/java/org/apache/atlas/repository/Constants.java d7f9c89 
>   repository/src/main/java/org/apache/atlas/repository/typestore/GraphBackedTypeStore.java
a94d157 
>   repository/src/main/java/org/apache/atlas/services/AtlasTypeAttributePatch.java PRE-CREATION

>   repository/src/main/java/org/apache/atlas/services/AtlasTypePatch.java PRE-CREATION

>   repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java 6a937f4

>   typesystem/src/main/java/org/apache/atlas/typesystem/types/AbstractDataType.java fad091d

>   typesystem/src/main/java/org/apache/atlas/typesystem/types/ClassType.java c56987a 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/EnumType.java bdd0a13 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/EnumTypeDefinition.java
6340615 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/HierarchicalType.java 7224699

>   typesystem/src/main/java/org/apache/atlas/typesystem/types/HierarchicalTypeDefinition.java
9a299f0 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/IDataType.java 85ddee7 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/StructType.java 6f40c1d

>   typesystem/src/main/java/org/apache/atlas/typesystem/types/StructTypeDefinition.java
f1ce1b7 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/TraitType.java f23bf5b 
>   typesystem/src/main/java/org/apache/atlas/typesystem/types/TypeSystem.java 70ba89b

>   typesystem/src/main/java/org/apache/atlas/typesystem/types/utils/TypesUtil.java ef8448d

>   typesystem/src/main/scala/org/apache/atlas/typesystem/json/TypesSerialization.scala
5618938 
> 
> Diff: https://reviews.apache.org/r/51939/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Sarath Kumar Subramanian
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message