----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51939/#review149157 -----------------------------------------------------------
Comments from partial review. I will complete the review later today. repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 55) <https://reviews.apache.org/r/51939/#comment216675> Consider moving 'private' methods to end of the class - after public and protected methods. It makes it easier to read/review. repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 56) <https://reviews.apache.org/r/51939/#comment216674> This method returns "false" when the given type has any of the attributes in the given array. checkIfAttributeExists() does not seem a good name. repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 100) <https://reviews.apache.org/r/51939/#comment216679> handle patchArray == null. Also review other places that deal with contents of patch file. repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 109) <https://reviews.apache.org/r/51939/#comment216681> "Invalid operation. Possible operation - ADD_ATTRIBUTE" ==> action + ": invalid action. Supported actions: ADD_ATTRIBUTE" repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 114) <https://reviews.apache.org/r/51939/#comment216685> It will be helpful to add a comment showing a sample patch processed by this method. repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 115) <https://reviews.apache.org/r/51939/#comment216682> metadataService is an attribute of this instance. Should this be passed as parameter? repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 116) <https://reviews.apache.org/r/51939/#comment216692> It might be useful to isolate patch details to a separate little class. Consider the following class hierarchy: class PatchData { String typeName; String applyToVersion; String updateToVersion; Map<String, Object> params; } abstract class AtlasTypePatch { public enum PatchResult { SUCCESS, FAILED, SKIPPED }; protected DefaultMetadataService metadataService; protected TypeSystem typeSystem; public void init(DefaultMetadataService metadataService, TypeSystem typeSystem) { this.metadataService = metadataService; this.typeSystem = typeSystem; } public abstract PatchResult applyPatch(PatchData patch); } class AddAttributePatch extends AtlasTypePatch { public PatchResult applyPatch(PatchData patch) { // .. } } repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 131) <https://reviews.apache.org/r/51939/#comment216683> currentTypeVersion.equalsIgnoreCase(applyVersion) || currentTypeVersion.startsWith(applyVersion)) ==> currentTypeVersion.equalsIgnoreCase(applyVersion) || currentTypeVersion.startsWith(applyVersion + ".")) ..to avoid applying policies for these values: currentVersion="1.11" with appyVersion=""1.1" repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 136) <https://reviews.apache.org/r/51939/#comment216688> LOG.error("failed to apply patch (typeName=" + typeName + "; applyToVersion=" + applyToVersion + "; updateToVersion=" + updateToVersion + "): type does not exist", e); repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 141) <https://reviews.apache.org/r/51939/#comment216686> Replace with: LOG.error("failed to apply patch (typeName=" + typeName + "; applyToVersion=" + applyToVersion + "; updateToVersion=" + updateToVersion + ")", e); Review other error messages as well. repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 148) <https://reviews.apache.org/r/51939/#comment216690> handle error and return null if the given type does not exist. repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.java (line 157) <https://reviews.apache.org/r/51939/#comment216693> Is using JSON string the only way to update types in the system? There is got to be a better way than use of opaque strings.. - Madhan Neethiraj On Sept. 15, 2016, 11:03 p.m., Sarath Kumar Subramanian wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51939/ > ----------------------------------------------------------- > > (Updated Sept. 15, 2016, 11:03 p.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/repository/Constants.java d7f9c89 > > repository/src/main/java/org/apache/atlas/repository/typestore/GraphBackedTypeStore.java > a94d157 > repository/src/main/java/org/apache/atlas/services/AtlasTypeUpdate.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 > >
