----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71568/#review218032 -----------------------------------------------------------
Some initial comments. I will make more comments later. addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java Lines 35 (patched) <https://reviews.apache.org/r/71568/#comment305519> How do we know the class name but need to load it via reflection? Where is this class defined and how is the JAR going to be included with Atlas at runtime? addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java Lines 49 (patched) <https://reviews.apache.org/r/71568/#comment305539> Is this style common in Atlas? I've seen it in Hive not sure about Atlas. If it is common why is it only present in this file and nowhere else? addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java Lines 54-60 (patched) <https://reviews.apache.org/r/71568/#comment305522> I think all of these could be placed into a Configuration class as opposed to bloating this class with configuration and functionality. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java Lines 65 (patched) <https://reviews.apache.org/r/71568/#comment305520> I think it is strange making references to closed source software (CDSW) in open source. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java Lines 94 (patched) <https://reviews.apache.org/r/71568/#comment305521> I think it could cause issues if we return here instead of exiting with a non-zero exit code. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java Lines 106 (patched) <https://reviews.apache.org/r/71568/#comment305507> I don't think this comment is useful. Add javadoc for this method if it is needed. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java Lines 141 (patched) <https://reviews.apache.org/r/71568/#comment305508> Remove comment or add java doc. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java Lines 223 (patched) <https://reviews.apache.org/r/71568/#comment305524> curl isn't installed on all machines by default. I think we should use the Java HTTP client to do this instead of spawning a separate process and using curl. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java Lines 226 (patched) <https://reviews.apache.org/r/71568/#comment305525> Are we sending the unencrypted username and password? I don't think that is very secure. Is there no other way to do this communication? addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/AtlasMlGovernanceProcessContext.java Lines 42 (patched) <https://reviews.apache.org/r/71568/#comment305527> nit: why is there a big space after this.hook? addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/AtlasMlGovernanceProcessContext.java Lines 75 (patched) <https://reviews.apache.org/r/71568/#comment305528> nit: this style is inconsistent. There are other methods shorter than these that are not in one line. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java Lines 54-58 (patched) <https://reviews.apache.org/r/71568/#comment305530> nit: incosistent style. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java Lines 65-66 (patched) <https://reviews.apache.org/r/71568/#comment305531> nit: incosistent style. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlDataBase.java Lines 27-28 (patched) <https://reviews.apache.org/r/71568/#comment305532> remove lombok addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModel.java Lines 33 (patched) <https://reviews.apache.org/r/71568/#comment305514> nit: incosistent style addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java Lines 33 (patched) <https://reviews.apache.org/r/71568/#comment305513> nit: inconsistent style in this class. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicaStatus.java Lines 43 (patched) <https://reviews.apache.org/r/71568/#comment305511> nit: inconsistent style addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicaStatus.java Lines 47-50 (patched) <https://reviews.apache.org/r/71568/#comment305512> nit: Some classes have a new line between the methods and others have them all compressed. Please choose a consistent style. addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicationPolicy.java Lines 40 (patched) <https://reviews.apache.org/r/71568/#comment305510> nit: style for this method is inconsistent addons/ml-bridge/src/test/resources/users-credentials.properties Lines 2-3 (patched) <https://reviews.apache.org/r/71568/#comment305538> Should we be pushing this information to an open source repository? Should we push this to a repository at all? pom.xml Lines 766 (patched) <https://reviews.apache.org/r/71568/#comment305506> I would avoid using lombok if it is not already present in the project. - Austin Nobis On Oct. 2, 2019, 11:31 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71568/ > ----------------------------------------------------------- > > (Updated Oct. 2, 2019, 11:31 p.m.) > > > Review request for atlas, Austin Nobis, Ashutosh Mestry, and Sarath > Subramanian. > > > Repository: atlas > > > Description > ------- > > add integration for ML > > > Diffs > ----- > > addons/hive-bridge/pom.xml 13384cb > addons/ml-bridge-shim/pom.xml PRE-CREATION > > addons/ml-bridge-shim/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java > PRE-CREATION > addons/ml-bridge/README.md PRE-CREATION > addons/ml-bridge/pom.xml PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/MlGovernanceHookTool.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/AtlasMlGovernanceProcessContext.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/GovernanceLineageHook.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/BaseMlGovernanceEvent.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/hooks/events/CreateMlGovernanceLineage.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlDataBase.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlDataType.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlLifecycleInfo.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModel.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelBuild.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelDeployment.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlModelRef.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlProject.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicaStatus.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicationPolicy.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlReplicationStatus.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlResources.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlTrainingData.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlTrainingDataEntry.java > PRE-CREATION > > addons/ml-bridge/src/main/java/org/apache/atlas/ml/governance/model/MlUser.java > PRE-CREATION > addons/ml-bridge/src/main/resources/atlas-log4j.xml PRE-CREATION > addons/ml-bridge/src/main/resources/governance_hook.sh PRE-CREATION > > addons/ml-bridge/src/test/java/org/apache/atlas/ml/governance/GovernanceHookIT.java > PRE-CREATION > > addons/ml-bridge/src/test/java/org/apache/atlas/ml/governance/GovernanceHookITBase.java > PRE-CREATION > addons/ml-bridge/src/test/resources/atlas-application.properties > PRE-CREATION > addons/ml-bridge/src/test/resources/atlas-log4j.xml PRE-CREATION > addons/ml-bridge/src/test/resources/cdswModelDeployments.json PRE-CREATION > addons/ml-bridge/src/test/resources/cdswModelDeploymentsTrainingData.json > PRE-CREATION > addons/ml-bridge/src/test/resources/cdswModels.json PRE-CREATION > addons/ml-bridge/src/test/resources/cdswModelsTrainingData.json > PRE-CREATION > addons/ml-bridge/src/test/resources/users-credentials.properties > PRE-CREATION > addons/ml-hook-api/pom.xml PRE-CREATION > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/Entity.java > PRE-CREATION > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/EntityType.java > PRE-CREATION > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/GovernanceHook.java > PRE-CREATION > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/HookContext.java > PRE-CREATION > > addons/ml-hook-api/src/main/java/org/apache/mlops/governance/hooks/MethodType.java > PRE-CREATION > addons/models/1000-Hadoop/1110-ml_model.json PRE-CREATION > pom.xml ce7d6fb > > > Diff: https://reviews.apache.org/r/71568/diff/2/ > > > Testing > ------- > > add integration tests > > > Thanks, > > Na Li > >
