----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/71426/#review217628 -----------------------------------------------------------
For easier readability, consider arranging methods/members in the following order: - static members: public, protected, private; within each accessor, ‘final’ members precede non-final - instance members: public, protected, private; within each accessor, ‘final’ members precede non-final - methods: public, protected, private; within each accessor, static methods precede non-static - methods: all constructors immediately after ‘public static’ methods - methods: all getters/setters immediately after constructors - methods: all @Override methods immediately after getter/setters docs/src/site/twiki/Import-API-Options.twiki Lines 142 (patched) <https://reviews.apache.org/r/71426/#comment304912> Please review and update the comment, per the current implementation. repository/src/main/java/org/apache/atlas/repository/impexp/ZipSourceWithBackingDirectory.java Lines 57 (patched) <https://reviews.apache.org/r/71426/#comment304919> Looking at the implementation, guidEntityJsonMap doesn't seem to have any entry for entity at all. It only has entries for typedef/creation-order/export-result. Consider getting rid of 'guidEntityJsonMap' and have members for above 3 entries. repository/src/main/java/org/apache/atlas/repository/impexp/ZipSourceWithBackingDirectory.java Lines 73 (patched) <https://reviews.apache.org/r/71426/#comment304915> - updateGuidZipEntryMap() => initGuidZipEntryMap() - consider moving #74 - #76 inside initGuidZipEntryMap() repository/src/main/java/org/apache/atlas/repository/impexp/ZipSourceWithBackingDirectory.java Lines 93 (patched) <https://reviews.apache.org/r/71426/#comment304916> add '@Override' to all methods that override base-class/interface. repository/src/main/java/org/apache/atlas/repository/impexp/ZipSourceWithBackingDirectory.java Lines 102 (patched) <https://reviews.apache.org/r/71426/#comment304917> Consider adding instance members for AtlasTypesDef and AtlasExportResult, creationOrder; and have them initialized in the constructor (or initGuidZipEntryMap()). repository/src/main/java/org/apache/atlas/repository/impexp/ZipSourceWithBackingDirectory.java Lines 118 (patched) <https://reviews.apache.org/r/71426/#comment304918> permissionChecks() is called from only from here; it will be easier to read if this call is replaced with its implementation: f.exists() && f.isDirectory() && f.canWrite() repository/src/test/java/org/apache/atlas/repository/impexp/ZipFileResourceTestUtils.java Line 96 (original), 99 (patched) <https://reviews.apache.org/r/71426/#comment304914> getZipSourceFrom() => getInputStreamFrom() webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java Line 395 (original), 394 (patched) <https://reviews.apache.org/r/71426/#comment304913> 'ZipSource' import, #45, is no more needed. Please review and remove. - Madhan Neethiraj On Sept. 6, 2019, 9:09 p.m., Ashutosh Mestry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/71426/ > ----------------------------------------------------------- > > (Updated Sept. 6, 2019, 9:09 p.m.) > > > Review request for atlas, Madhan Neethiraj, Nikhil Bonte, Nixon Rodrigues, > and Sarath Subramanian. > > > Bugs: ATLAS-3396 > https://issues.apache.org/jira/browse/ATLAS-3396 > > > Repository: atlas > > > Description > ------- > > **Background** > The approach adds another option to _ImportService_ to be able to optionally > use a directory on the server for storing data to be imported. > > **Approach** > This is a backward compatible. > > - - New: _ZipSourceWithBackingDirectory_: Uses a temporary directory for > storing contents of the zip file. > - Modified: _ImportService_ Now takes _InputStream_ as parameter instead of > _ZipSource_. > - Modified: _ImportRequest_ Additionl option takes _backingDirectory_ as > additional option. Specifying this will use the new > _ZipSourceWithBackingDirectory_ during import. > > > **CURL** > > Use pre-configured temporary directory, set _atlas.import.temporaryDirectory_ > in the applicatino properties. > > _CURL_ > ``` > curl -v -X POST -u admin:admin -H "Content-Type: multipart/form-data" -H > "Cache-Control: no-cache" -F request=@../docs/import-options.json -F > data=@../docs/smalldb.zip http://localhost:21000/api/atlas/admin/import > ``` > > **Documentation** > Updated. > > > Diffs > ----- > > distro/src/conf/atlas-application.properties 654a9304e > docs/src/site/twiki/Import-API-Options.twiki 4004e7013 > > repository/src/main/java/org/apache/atlas/repository/impexp/ImportService.java > b5d8b7c39 > > repository/src/main/java/org/apache/atlas/repository/impexp/ZipSourceWithBackingDirectory.java > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/AtlasEntityStreamForImport.java > 90ae15d1e > > repository/src/main/java/org/apache/atlas/repository/store/graph/v1/EntityImportStream.java > d4b6c5505 > > repository/src/test/java/org/apache/atlas/repository/impexp/ExportIncrementalTest.java > a355297a8 > > repository/src/test/java/org/apache/atlas/repository/impexp/ExportSkipLineageTest.java > eaf4602d5 > > repository/src/test/java/org/apache/atlas/repository/impexp/ImportServiceTest.java > 693a163f8 > > repository/src/test/java/org/apache/atlas/repository/impexp/ReplicationEntityAttributeTest.java > 868b732d3 > > repository/src/test/java/org/apache/atlas/repository/impexp/ZipFileResourceTestUtils.java > a2a5f58dc > webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java > 8417e7eb0 > > > Diff: https://reviews.apache.org/r/71426/diff/5/ > > > Testing > ------- > > **Unit tests** > Updated to exercise the new implementation. > > **Volume tests** > Imported 4GB ZIP file that took 26 hours. Memory and CPU stayed in normal > range during this operation. > > **Functional tests** > Accuracy tests preformed on small size data. > > **Pre-commit Build** > https://builds.apache.org/view/A/view/Atlas/job/PreCommit-ATLAS-Build-Test/1380/ > > > Thanks, > > Ashutosh Mestry > >
