----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70512/#review215010 -----------------------------------------------------------
addons/impala-bridge-shim/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 35 (patched) <https://reviews.apache.org/r/70512/#comment301308> Shim is not needed for ImpalaLineageTool. It is necessary only for the classes that run within host components (like Hive, HBase). Please remove this class. addons/impala-bridge/pom.xml Lines 47 (patched) <https://reviews.apache.org/r/70512/#comment301307> Following libraries are not needed, as the implementation uses notifications, instead of REST APIs. Please review and remove these and other unused libraries from here: - atlas-client-v1 - atlas-client-v2 - hdfs-model addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 52 (patched) <https://reviews.apache.org/r/70512/#comment301310> 'implements Runnable' seems unnecessary here, as no threads are created to run the tool. addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 54 (patched) <https://reviews.apache.org/r/70512/#comment301312> It may not be useful to designate a 'default-directoy'; the tool should process the files specified in the command-line. Please consider removing this. Anyway, this doesn't seem to be used at all. Please review. addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 59 (patched) <https://reviews.apache.org/r/70512/#comment301309> Instead of using 'static' fields (args, instance), consider using class members and instantiate the class from main(). public class ImpalaLineageTool { public static void main(String[] args) { ImpalaLineageTool instance = new ImpalaLineageTool(args); instance.run(); } private ImpalaLineageTool(String[] args) { this.args = args; } public void run() { // .. } } addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 61 (patched) <https://reviews.apache.org/r/70512/#comment301311> Consider avoiding test specific details (like 'isUnitTest') from product code. addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 80 (patched) <https://reviews.apache.org/r/70512/#comment301317> log message seems incorrect. Please review the implementation to make sure that current directoy is read when arguments are not provided. addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 87 (patched) <https://reviews.apache.org/r/70512/#comment301315> - args.length may be less than 2 here - see line #79 above - instead of using args[1] here, consider initializing following variable after line #83 and use it here: String directoryName = args[1]; addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 91 (patched) <https://reviews.apache.org/r/70512/#comment301314> info level log will be useful here. addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 123 (patched) <https://reviews.apache.org/r/70512/#comment301318> It will help to use consistent stype in use of '{'. Please consider moving '{' to end of previous line. And review other such occurrences as well. addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 124 (patched) <https://reviews.apache.org/r/70512/#comment301319> It will be useful to include name of the file in the log: LOG.info("deleted lineage file {}", currentFile.getAbsolutePath()); LOG.info("failed to delete lineage file {}", currentFile.getAbsolutePath()); Please review and update other such occurrences. addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 167 (patched) <https://reviews.apache.org/r/70512/#comment301316> info level log would be useful here. addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java Lines 171 (patched) <https://reviews.apache.org/r/70512/#comment301313> Please handle 'listOfFiles == null' condition. addons/impala-bridge/src/main/java/org.apache.atlas.impala/model/ImpalaDataTypes.java Lines 21 (patched) <https://reviews.apache.org/r/70512/#comment301305> ImpalaDataTypes doesn't seem to be used. If true, please consider removing this enum. addons/impala-bridge/src/main/java/org.apache.atlas.impala/model/LineageQuery.java Lines 35 (patched) <https://reviews.apache.org/r/70512/#comment301306> Consider renaming LineageQuery to ImpalaQuery or ImpalaProcess addons/impala-bridge/src/main/resources/import-impala.sh Lines 48 (patched) <https://reviews.apache.org/r/70512/#comment301304> Avoid references to paths like /usr/hdp/. Instead consider using paths relative to the script directoty i.e. ${BASEDIR}. - Madhan Neethiraj On May 2, 2019, 8:31 p.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/70512/ > ----------------------------------------------------------- > > (Updated May 2, 2019, 8:31 p.m.) > > > Review request for atlas, Ashutosh Mestry, Aadarsh Jajodia, madhan, Sarath > Subramanian, and Xinran Tinney. > > > Repository: atlas > > > Description > ------- > > Impala generates lineage records for its commands. This new feature will read > Impala lineage file, convert the lineage record to Atlas entities and send > them to Atlas. In this way, Atlas can get lineage of Impala operation. > > The metadata referred in the lineage are captured in Hive Metastore hook and > sent to Atlas. This work is done in ATLAS-3148 > > This jira only supports the Impala command "create view". Following jira will > add support for more Impala commands. > > > Diffs > ----- > > addons/impala-bridge-shim/pom.xml PRE-CREATION > > addons/impala-bridge-shim/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > PRE-CREATION > addons/impala-bridge/pom.xml PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/hook/AtlasImpalaHookContext.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/hook/ImpalaLineageHook.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/hook/ImpalaOperationParser.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/hook/events/BaseImpalaEvent.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/hook/events/CreateImpalaProcess.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/model/IImpalaLineageHook.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/model/ImpalaDataTypes.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/model/ImpalaNode.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/model/ImpalaOperationType.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/model/LineageEdge.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/model/LineageQuery.java > PRE-CREATION > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/model/LineageVertex.java > PRE-CREATION > addons/impala-bridge/src/main/resources/atlas-log4j.xml PRE-CREATION > addons/impala-bridge/src/main/resources/import-impala.sh PRE-CREATION > > addons/impala-bridge/src/test/java/org/apache/atlas/impala/ImpalaLineageITBase.java > PRE-CREATION > > addons/impala-bridge/src/test/java/org/apache/atlas/impala/ImpalaLineageToolIT.java > PRE-CREATION > > addons/impala-bridge/src/test/java/org/apache/atlas/impala/hook/ImpalaLineageHookIT.java > PRE-CREATION > addons/impala-bridge/src/test/resources/atlas-application.properties > PRE-CREATION > addons/impala-bridge/src/test/resources/atlas-log4j.xml PRE-CREATION > addons/impala-bridge/src/test/resources/hive-site.xml PRE-CREATION > addons/impala-bridge/src/test/resources/impala1.json PRE-CREATION > addons/impala-bridge/src/test/resources/impala2.json PRE-CREATION > addons/impala-bridge/src/test/resources/impala3.json PRE-CREATION > addons/impala-bridge/src/test/resources/users-credentials.properties > PRE-CREATION > pom.xml 7de5d31 > > > Diff: https://reviews.apache.org/r/70512/diff/13/ > > > Testing > ------- > > Run the tool in real cluster that has Atlas server with Impala lineage file > as input for creating view. The Atlas UI displays hive_lineage lineage and > hive_column_lineage. > Add new integration tests and they pass > > > Thanks, > > Na Li > >
