> On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge-shim/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 35 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142460#file2142460line35> > > > > 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.
done > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/pom.xml > > Lines 47 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142461#file2142461line47> > > > > 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 I need atlas-client-v2 for testing (fetching process entity from Atlas server). I mark the scope of libraries I need as "test" > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 52 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142462#file2142462line52> > > > > 'implements Runnable' seems unnecessary here, as no threads are created > > to run the tool. removed Runnable > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 59 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142462#file2142462line59> > > > > 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() { > > // .. > > } > > } done. > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 61 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142462#file2142462line61> > > > > Consider avoiding test specific details (like 'isUnitTest') from > > product code. removed > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 80 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142462#file2142462line80> > > > > log message seems incorrect. Please review the implementation to make > > sure that current directoy is read when arguments are not provided. I changed the code to return if the desired number of input arguments are not provided > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 87 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142462#file2142462line87> > > > > - 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]; I add directoryName as instance variable, which are set in getCurrentFiles(args);. So following processing can use it directly. > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 91 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142462#file2142462line91> > > > > info level log will be useful here. changed to LOG.info > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 123 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142462#file2142462line123> > > > > 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. done > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 124 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142462#file2142462line124> > > > > 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. added names > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 167 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142462#file2142462line167> > > > > info level log would be useful here. done > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/ImpalaLineageTool.java > > Lines 171 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142462#file2142462line171> > > > > Please handle 'listOfFiles == null' condition. done > On May 3, 2019, 2:01 a.m., Madhan Neethiraj wrote: > > addons/impala-bridge/src/main/java/org.apache.atlas.impala/model/ImpalaDataTypes.java > > Lines 21 (patched) > > <https://reviews.apache.org/r/70512/diff/13/?file=2142469#file2142469line21> > > > > ImpalaDataTypes doesn't seem to be used. If true, please consider > > removing this enum. removed - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/70512/#review215010 ----------------------------------------------------------- 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 > >
