You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Madhan Neethiraj <ma...@apache.org> on 2020/10/12 07:21:17 UTC
Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved
Resiliency
-----------------------------------------------------------
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
>
>
Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved
Resiliency
Posted by Ashutosh Mestry via Review Board <no...@reviews.apache.org>.
> On Oct. 12, 2020, 7:21 a.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/notification/LogConfigUtils.java
> > Lines 48 (patched)
> > <https://reviews.apache.org/r/72893/diff/5/?file=2240466#file2240466line48>
> >
> > 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".
Any approach to infer log directory is not yielding predictable results. The cluster manager chages the appenders. In some cases redactors are applied, others are replaced with ConsoleAppenders.
The approach above works predictably only with Hive and Atlas.
- Ashutosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72893/#review222012
-----------------------------------------------------------
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.
> - HBase.
> - Spark.
>
>
> Diffs
> -----
>
> addons/hive-bridge/src/main/java/org/apache/atlas/hive/hook/HiveHook.java 651323490
> 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/FileOperations.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/local/FileLockedReadWrite.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/spool/utils/local/FileOpAppend.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/spool/utils/local/FileOpCompaction.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/spool/utils/local/FileOpDelete.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/spool/utils/local/FileOpRead.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/spool/utils/local/FileOpUpdate.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/notification/spool/utils/local/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
> pom.xml 6b5d2fde6
>
>
> Diff: https://reviews.apache.org/r/72893/diff/6/
>
>
> 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
>
>