----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/72893/#review222012 -----------------------------------------------------------
notification/pom.xml Lines 61 (patched) <https://reviews.apache.org/r/72893/#comment311073> Consider replacing version=2.8 here with a property in root pom.xml. notification/src/main/java/org/apache/atlas/notification/LogConfigUtils.java Lines 48 (patched) <https://reviews.apache.org/r/72893/#comment311074> Atlas log4j configuration is likely to have multiple appenders to file - FILE, LARGE_FILE, AUDIT, METRICS, FAILED, perf_appender. To be consistent/predictable, I suggest to look for appender named "FILE". notification/src/main/java/org/apache/atlas/notification/spool/Archiver.java Lines 36 (patched) <https://reviews.apache.org/r/72893/#comment311075> Consider marking all members as final, as these are not updated after initialization in the constructor. notification/src/main/java/org/apache/atlas/notification/spool/Archiver.java Lines 53 (patched) <https://reviews.apache.org/r/72893/#comment311076> moveToArchiveDir() is called only from archive() method above, which is not static. I suggest to remove 'static' from this method. And also remove parameters source and archiveFilter - as these are already available as instance members. notification/src/main/java/org/apache/atlas/notification/spool/Archiver.java Lines 63 (patched) <https://reviews.apache.org/r/72893/#comment311078> Line #63 should be before the call to FileUtils.moveFile() above? notification/src/main/java/org/apache/atlas/notification/spool/Archiver.java Lines 72 (patched) <https://reviews.apache.org/r/72893/#comment311077> removeOldFiles() is called only from archive() method above, which is not static. I suggest to: - mark this method as private - remove 'static' from this method - remove parameters source and archiveFilter - as these are already available as instance members notification/src/main/java/org/apache/atlas/notification/spool/AtlasFileSpool.java Lines 54 (patched) <https://reviews.apache.org/r/72893/#comment311079> return from this method doesn't seem to be used. Consider changing return to void, and throwing an exception in case of failure. notification/src/main/java/org/apache/atlas/notification/spool/AtlasFileSpool.java Lines 119 (patched) <https://reviews.apache.org/r/72893/#comment311081> isInitDone() => hasInitSucceeded() notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java Lines 57 (patched) <https://reviews.apache.org/r/72893/#comment311080> Instead of returning a boolean from setup(), consider throwing an exception with appropriate message. - Madhan Neethiraj On Oct. 6, 2020, 5:47 p.m., Ashutosh Mestry wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/72893/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2020, 5:47 p.m.) > > > Review request for atlas, Deep Singh, Madhan Neethiraj, mayank jain, Nikhil > Bonte, Nixon Rodrigues, and Sarath Subramanian. > > > Bugs: ATLAS-3427 > https://issues.apache.org/jira/browse/ATLAS-3427 > > > Repository: atlas > > > Description > ------- > > (Internal review) > **Description** > Integration: _AtlasHook_ receives enhanced _NotificationInterface_ in the > form of _AtlasFileSpool_. This capability can be optionally set using: > _atlas.hook.spool.dir_ configuration property. > > _AtlasFileSpool_: Enhances default hook functionality by encapsulating the > default _NotificationInterface_ with file spooling capability. In case of > destination being unavailable, the messages received will be spooled to local > file system. > > What is spool? > - These are files containg data (messages in this specific case). > > Files on the disk that are managed using index files. Index files have > special structure that is used to describe the spool files. > > Who generates spool files? > _AtlasHook_ is base class from which all hooks. It is responsible for sending > messages to a destination (mostly Kafka). If destination is down, an > exception is raised by the destination. Before the implementation, the > message was logged after retry. With this implementation, the message will be > spooled, and when the destination is up, it is sent to the destination. > > How are spool file stored? > - Spool files and index files are store on disk in local file system. > > Structure of _AtlasFileSpool_: > - _IndexManagement_: Storage and retrieval of index files. Provides Spooler > (see below) with _PrintWriter_ to write messages to the disk. > - _Spooler_: > - Interacts with _IndexManagement_ to receive _PrintWriter_. > - Serializes messages and writes to the spool file. > - _Publisher_: > - Interacts with _IndexManagement_ to receive _IndexRecord_ that is ready > to be published. > - Reads messages from the spool file that is described in the _IndexRecord_ > and attempts to send it to the destination. > - If destination is down, it waits for destination to be up. > - _SpoolConfiguration_: Stores configuration specific to the implementation. > - _SpoolUtils_: Utility methods for file storage. > > > **Impacted Areas** > Hooks: > - Hive: HS2 > - Hive: HMS > - Impala > > > Diffs > ----- > > > addons/hive-bridge-shim/src/main/java/org/apache/atlas/hive/hook/HiveHook.java > 2a4d067e5 > > addons/hive-bridge/src/main/java/org/apache/atlas/hive/hook/HiveMetastoreHookImpl.java > 3c0f0c106 > notification/pom.xml 8affd59a2 > notification/src/main/java/org/apache/atlas/hook/AtlasHook.java 8659126eb > notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java > b319e81b8 > notification/src/main/java/org/apache/atlas/kafka/NotificationProvider.java > 2dd970ef7 > > notification/src/main/java/org/apache/atlas/notification/AbstractNotification.java > 45a66bf07 > > notification/src/main/java/org/apache/atlas/notification/LogConfigUtils.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/NotificationException.java > 2dd9c9fa0 > > notification/src/main/java/org/apache/atlas/notification/NotificationInterface.java > 6caf7e2d5 > > notification/src/main/java/org/apache/atlas/notification/spool/Archiver.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/AtlasFileSpool.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/Publisher.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/SpoolConfiguration.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/SpoolUtils.java > PRE-CREATION > notification/src/main/java/org/apache/atlas/notification/spool/Spooler.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/models/IndexRecord.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/models/IndexRecords.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/utils/FileLockedRead.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/utils/FileOpAppend.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/utils/FileOpCompaction.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/utils/FileOpDelete.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/utils/FileOpRead.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/utils/FileOpUpdate.java > PRE-CREATION > > notification/src/main/java/org/apache/atlas/notification/spool/utils/FileOperation.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/AbstractNotificationTest.java > 94cb70d20 > > notification/src/test/java/org/apache/atlas/notification/spool/AtlasFileSpoolTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/spool/BaseTest.java > PRE-CREATION > > notification/src/test/java/org/apache/atlas/notification/spool/IndexFileManagerTest.java > PRE-CREATION > notification/src/test/resources/spool/index-test-src-1.json PRE-CREATION > notification/src/test/resources/spool/index-test-src-1_closed.json > PRE-CREATION > > repository/src/main/java/org/apache/atlas/repository/patches/ClassificationTextPatch.java > 835194277 > > repository/src/main/java/org/apache/atlas/repository/patches/ConcurrentPatchProcessor.java > 5a9ac2abe > > > Diff: https://reviews.apache.org/r/72893/diff/5/ > > > Testing > ------- > > **Unit testing** > Additional unit tests added. > > **System Testing** > End-to-end verification for each of the hooks. > > **Volume Testing** > Spooled large data and verified publishing. Ensured that sequence of messages > is maintained. > > > Thanks, > > Ashutosh Mestry > >
