----------------------------------------------------------- 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 > >
