Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved Resiliency

2020-10-22 Thread Ashutosh Mestry via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72893/
---

(Updated Oct. 22, 2020, 9:05 p.m.)


Review request for atlas, Deep Singh, Madhan Neethiraj, mayank jain, Nikhil 
Bonte, Nixon Rodrigues, and Sarath Subramanian.


Changes
---

Updates include: 
- Added property for enabling/disabling the feature.


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 (updated)
-

  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

Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved Resiliency

2020-10-21 Thread Ashutosh Mestry via Review Board 


> On Oct. 20, 2020, 6:59 p.m., Sarath Subramanian wrote:
> > notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
> > Lines 59 (patched)
> > 
> >
> > consider replacing exceptions in line 59, 64, 69, 74 with atlas base 
> > exception with error code.

Reason for keeping it unchanged is that _AtlasBaseException_ has HTTP error 
code, making it more appropriate for cases where exceptions has to be thrown as 
part of REST API.


> On Oct. 20, 2020, 6:59 p.m., Sarath Subramanian wrote:
> > notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
> > Lines 89 (patched)
> > 
> >
> > consider initializing within the constructor and remove lines 93-95

Reason for using 2-phase creation is that the source information is not 
available at the time of creation. It becomes available only after the ctor of 
the hook is called.


> On Oct. 20, 2020, 6:59 p.m., Sarath Subramanian wrote:
> > notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
> > Lines 180 (patched)
> > 
> >
> > consider calling init() within the constructor

Same reason as above.


> On Oct. 20, 2020, 6:59 p.m., Sarath Subramanian wrote:
> > notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
> > Lines 321 (patched)
> > 
> >
> > consider calling init() within the constructor

Same reason as above.


> On Oct. 20, 2020, 6:59 p.m., Sarath Subramanian wrote:
> > notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
> > Lines 401 (patched)
> > 
> >
> > consider calling init() within the constructor

Same reason as above.


> On Oct. 20, 2020, 6:59 p.m., Sarath Subramanian wrote:
> > notification/src/main/java/org/apache/atlas/notification/spool/models/IndexRecord.java
> > Lines 59 (patched)
> > 
> >
> > consider using enum to represent index record status

I had it as enum before, only to realize that I ended up changing it to string 
almost all the time. Hence decided to remove it as ENUM.


> On Oct. 20, 2020, 6:59 p.m., Sarath Subramanian wrote:
> > pom.xml
> > Lines 700 (patched)
> > 
> >
> > log4j.version property is duplicated in line# 688, consider updating 
> > the previous log4j version.

We are working with 2 version of log4j.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72893/#review222078
---


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

Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved Resiliency

2020-10-21 Thread Ashutosh Mestry via Review Board 


> On Oct. 20, 2020, 7:19 p.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
> > Lines 351 (patched)
> > 
> >
> > Exception OverlappingFileLockException doesn't seem to be thrown in the 
> > try block. What condition causes this exception?

Good question: This exception is caused when a process attempt to lock a record 
that is already locked by some other process.


> On Oct. 20, 2020, 7:19 p.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/notification/spool/Publisher.java
> > Lines 150 (patched)
> > 
> >
> > Should 'messages' be cleared after #150, since dispatch() is called 
> > again with 'messages' at line #154 below.

dispatch method clears it in the finally block.


> On Oct. 20, 2020, 7:19 p.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/notification/spool/SpoolUtils.java
> > Lines 140 (patched)
> > 
> >
> > SPOOL_FILE_NAME_FORMAT_PREFIX is defined as "%s.%s%s"; the second 
> > argument here is an int. Please review and update the format string, if 
> > necessary.

This format is no longer used. I was able to remove this.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72893/#review222077
---


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/ma

Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved Resiliency

2020-10-20 Thread Madhan Neethiraj 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72893/#review222079
---


Ship it!




Ship It!

- 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.
> - 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

Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved Resiliency

2020-10-20 Thread Madhan Neethiraj 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72893/#review222077
---




notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
Lines 351 (patched)


Exception OverlappingFileLockException doesn't seem to be thrown in the try 
block. What condition causes this exception?



notification/src/main/java/org/apache/atlas/notification/spool/Publisher.java
Lines 150 (patched)


Should 'messages' be cleared after #150, since dispatch() is called again 
with 'messages' at line #154 below.



notification/src/main/java/org/apache/atlas/notification/spool/SpoolUtils.java
Lines 140 (patched)


SPOOL_FILE_NAME_FORMAT_PREFIX is defined as "%s.%s%s"; the second argument 
here is an int. Please review and update the format string, if necessary.


- 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.
> - 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/a

Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved Resiliency

2020-10-20 Thread Sarath Subramanian 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72893/#review222078
---




notification/src/main/java/org/apache/atlas/notification/LogConfigUtils.java
Lines 70 (patched)


consider moving the logic to get file appender path from approach2 within 
getFileAppenderPath()

something like in line #70:

if (StringUtils.isEmpty(ret)) {
  // move getFileAppenderPathApproach2() implementation here.
}



notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
Lines 59 (patched)


consider replacing exceptions in line 59, 64, 69, 74 with atlas base 
exception with error code.



notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
Lines 89 (patched)


consider initializing within the constructor and remove lines 93-95



notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
Lines 180 (patched)


consider calling init() within the constructor



notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
Lines 321 (patched)


consider calling init() within the constructor



notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
Lines 401 (patched)


consider calling init() within the constructor



notification/src/main/java/org/apache/atlas/notification/spool/models/IndexRecord.java
Lines 59 (patched)


consider using enum to represent index record status



pom.xml
Lines 700 (patched)


log4j.version property is duplicated in line# 688, consider updating the 
previous log4j version.


- Sarath Subramanian


On Oct. 6, 2020, 10:47 a.m., Ashutosh Mestry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/72893/
> ---
> 
> (Updated Oct. 6, 2020, 10:47 a.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 
>   
>

Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved Resiliency

2020-10-20 Thread Nixon Rodrigues 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72893/#review222076
---


Ship it!




Ship It!

- Nixon Rodrigues


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/

Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved Resiliency

2020-10-14 Thread Ashutosh Mestry via Review Board 


> On Oct. 12, 2020, 7:21 a.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/notification/LogConfigUtils.java
> > Lines 48 (patched)
> > 
> >
> > 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/

Re: Review Request 72893: ATLAS-3427: Hook Enhancements for Improved Resiliency

2020-10-12 Thread Madhan Neethiraj 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/72893/#review222012
---




notification/pom.xml
Lines 61 (patched)


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)


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)


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)


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)


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)


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)


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)


isInitDone() => hasInitSucceeded()



notification/src/main/java/org/apache/atlas/notification/spool/IndexManagement.java
Lines 57 (patched)


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