----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70913/#review216027 -----------------------------------------------------------
addons/hive-bridge/src/main/java/org/apache/atlas/hive/hook/events/CreateHiveProcess.java Lines 139 (patched) <https://reviews.apache.org/r/70913/#comment302980> Is addition of 'createTime' attribute (in CreateHiveProcess and CreateTable) related to this enhancement (of dynamic-attribute)? If not, please consider a seperate patch for these changes. addons/models/1000-Hadoop/1030-hive_model.json Lines 89 (patched) <https://reviews.apache.org/r/70913/#comment302981> 'outputs' is an array, hence 'qualifiedName' on this doesn't look correct. I suggest to only use non-array attributes in dynamic-expressions. intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java Lines 674 (patched) <https://reviews.apache.org/r/70913/#comment302998> - consider replacing literal string "dynAttribute" with a 'static final' - like: private static final DYN_ATTR_PREFIX = "dynAttribute:"; - consider replacing substring(..).equals() with: s.startsWith(DYN_ATTR_PREFIX) - consider moving the logic in fillDynFlagsInfo() method to AtlasAttribute constuction, entityDef.getOptions() should be passed to AtlasAttribute constructor for this. intg/src/main/java/org/apache/atlas/type/AtlasStructType.java Lines 713 (patched) <https://reviews.apache.org/r/70913/#comment302993> Please consider explicitly initializing these to 'false' - for better readability. intg/src/main/java/org/apache/atlas/type/AtlasStructType.java Line 714 (original), 716 (patched) <https://reviews.apache.org/r/70913/#comment302983> Please retain existing constructor, and add a new one with the additional parameters - for backward compatibility. Same for line #744 as well. intg/src/main/java/org/apache/atlas/type/AtlasStructType.java Lines 828 (patched) <https://reviews.apache.org/r/70913/#comment302995> Isn't possible to determine if this attribute is 'dyn' - by looking into entityDef.options passed to constructor (see other comment in AtlasEntityType)? Please consider initializing isDynAttribute in the constructor and mark it as final. setIsDynAttribute() shouldn't be needed any more. intg/src/main/java/org/apache/atlas/type/AtlasStructType.java Lines 914 (patched) <https://reviews.apache.org/r/70913/#comment302991> 1. rename: fillDynFlags -> updateDynFlags 2. parameter names are not intutive; are they attributes of current entity-def? if yes, a better place for this method would be AtlasEntityType - like: class AtlasEntityType { private void setDynEvalTriggerAttributes(List<String> attrNames) { ... } } if #2 above makes sense, changes in AtlasAttribute constructor may not be necessary. repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java Line 305 (original) <https://reviews.apache.org/r/70913/#comment302979> Consider avoiding removing line seperators, like #305, #317, #327. Also, only changes in this files are white-space/comments related. Consider not including this file in review. - Madhan Neethiraj On June 20, 2019, 11:52 p.m., Merryle Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70913/ > ----------------------------------------------------------- > > (Updated June 20, 2019, 11:52 p.m.) > > > Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, Sridhar K, Le Ma, > Madhan Neethiraj, and Sarath Subramanian. > > > Repository: atlas > > > Description > ------- > > ATLAS-3286: Populated dynamic attribute flags for AtlasAttributes > > > Diffs > ----- > > > addons/hive-bridge/src/main/java/org/apache/atlas/hive/hook/events/CreateHiveProcess.java > 7791fb4c656a354065ad8f28ae8618c6e07d8ed4 > > addons/hive-bridge/src/main/java/org/apache/atlas/hive/hook/events/CreateTable.java > aedb1552be14bf92d3464f44a5d89e8106104aeb > addons/models/1000-Hadoop/1030-hive_model.json > 8901aa4aa86fb2802a9e9b1e65c2ff8aad8855ad > intg/src/main/java/org/apache/atlas/type/AtlasClassificationType.java > 4171942026efbc021d707fdc047e4d647208d021 > intg/src/main/java/org/apache/atlas/type/AtlasEntityType.java > d9ae9e36773146fad652a1e28fc8822bae5c8557 > intg/src/main/java/org/apache/atlas/type/AtlasRelationshipType.java > 98071b297efef8596206842923d85d20b267cc10 > intg/src/main/java/org/apache/atlas/type/AtlasStructType.java > 254eee4c9d18ab87a8862ce479fc2274227c7c29 > > repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphMapper.java > 64f5503fe70ebd494cabecd04da78494b6b827ac > > repository/src/main/java/org/apache/atlas/repository/store/graph/v2/EntityGraphRetriever.java > 02638c3ba5c429380ec9f098c143b49314604159 > > > Diff: https://reviews.apache.org/r/70913/diff/1/ > > > Testing > ------- > > Planning to add unit tests > > > Thanks, > > Merryle Wang > >
