You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@atlas.apache.org by Hemanth Yamijala <yh...@gmail.com> on 2016/06/15 10:11:57 UTC
Review Request 48723: ATLAS-901 Log messages that cannot be sent to
Kafka to a specific log configuration
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48723/
-----------------------------------------------------------
Review request for atlas.
Bugs: ATLAS-901
https://issues.apache.org/jira/browse/ATLAS-901
Repository: atlas
Description
-------
The description of the approach is documented on JIRA here: https://issues.apache.org/jira/browse/ATLAS-901?focusedCommentId=15331480&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15331480
Diffs
-----
notification/src/main/java/org/apache/atlas/hook/AtlasHook.java 71029b0
notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java PRE-CREATION
notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 1ee62d2
notification/src/main/java/org/apache/atlas/notification/NotificationException.java d9d89df
notification/src/test/java/org/apache/atlas/hook/AtlasHookTest.java 16cb0f0
Diff: https://reviews.apache.org/r/48723/diff/
Testing
-------
Tested by integrating with hive hook. Brought down Kafka when the hook has to send messages. Ensured that the message is logged into the log file.
Added some UTs for the changes.
Thanks,
Hemanth Yamijala
Re: Review Request 48723: ATLAS-901 Log messages that cannot be sent
to Kafka to a specific log configuration
Posted by Madhan Neethiraj <ma...@apache.org>.
> On June 16, 2016, 6:58 a.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java, line 57
> > <https://reviews.apache.org/r/48723/diff/1/?file=1419538#file1419538line57>
> >
> > Consider obtaining the logfile name from configuration - similar to flag "atlas.notification.log.failed.messages" in AtlasHook.java.
> >
> > This will enable users to customize the log file location per deployment needs. The default log file can be in directory System.getProperty("atlas.log.dir").
Since this runs the host component process, "atlas.log.dir" would not be set. Please ignore this suggestion; instead we can enable failed messages logging only if the log file location is specified in the configuration - in a property named like "atlas.hook.notification.failed.messages.filename"
- Madhan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48723/#review137927
-----------------------------------------------------------
On June 15, 2016, 10:11 a.m., Hemanth Yamijala wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48723/
> -----------------------------------------------------------
>
> (Updated June 15, 2016, 10:11 a.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-901
> https://issues.apache.org/jira/browse/ATLAS-901
>
>
> Repository: atlas
>
>
> Description
> -------
>
> The description of the approach is documented on JIRA here: https://issues.apache.org/jira/browse/ATLAS-901?focusedCommentId=15331480&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15331480
>
>
> Diffs
> -----
>
> notification/src/main/java/org/apache/atlas/hook/AtlasHook.java 71029b0
> notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 1ee62d2
> notification/src/main/java/org/apache/atlas/notification/NotificationException.java d9d89df
> notification/src/test/java/org/apache/atlas/hook/AtlasHookTest.java 16cb0f0
>
> Diff: https://reviews.apache.org/r/48723/diff/
>
>
> Testing
> -------
>
> Tested by integrating with hive hook. Brought down Kafka when the hook has to send messages. Ensured that the message is logged into the log file.
>
> Added some UTs for the changes.
>
>
> Thanks,
>
> Hemanth Yamijala
>
>
Re: Review Request 48723: ATLAS-901 Log messages that cannot be sent
to Kafka to a specific log configuration
Posted by Hemanth Yamijala <yh...@gmail.com>.
> On June 16, 2016, 6:58 a.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/hook/AtlasHook.java, line 123
> > <https://reviews.apache.org/r/48723/diff/1/?file=1419537#file1419537line123>
> >
> > if 'messages' contains multiple messages, notification processing seems to stop at the first failure - in KafkaNotification.java. Logging here only writes the failed message; subsequent message in the list will not be logged. Consider NotificationException to store list of messages not sent; and update the logging here to write all these messages.
Done. I am collecting all failed messages into NotificationException and logging all the errors to the failed messages log. Added new tests for multiple messages.
> On June 16, 2016, 6:58 a.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java, line 57
> > <https://reviews.apache.org/r/48723/diff/1/?file=1419538#file1419538line57>
> >
> > Consider obtaining the logfile name from configuration - similar to flag "atlas.notification.log.failed.messages" in AtlasHook.java.
> >
> > This will enable users to customize the log file location per deployment needs. The default log file can be in directory System.getProperty("atlas.log.dir").
>
> Madhan Neethiraj wrote:
> Since this runs the host component process, "atlas.log.dir" would not be set. Please ignore this suggestion; instead we can enable failed messages logging only if the log file location is specified in the configuration - in a property named like "atlas.hook.notification.failed.messages.filename"
I have made the log file name configurable. However, a slight change to your proposal: I did retain the boolean to check if the logging is necessary or not. My rationale was mainly to have a default value of true for logging failed messages in the interest of increased resilience. Having a log file with no default value would have prevented this. And having a default value for the file would have meant it is logged always (if we use the presence of the value as an indicator for logging). Please let me know if you feel strongly about having just one configuration. I will leave the issue open to hear back from you.
- Hemanth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48723/#review137927
-----------------------------------------------------------
On June 16, 2016, 12:21 p.m., Hemanth Yamijala wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48723/
> -----------------------------------------------------------
>
> (Updated June 16, 2016, 12:21 p.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-901
> https://issues.apache.org/jira/browse/ATLAS-901
>
>
> Repository: atlas
>
>
> Description
> -------
>
> The description of the approach is documented on JIRA here: https://issues.apache.org/jira/browse/ATLAS-901?focusedCommentId=15331480&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15331480
>
>
> Diffs
> -----
>
> docs/src/site/twiki/Configuration.twiki 0e122fe
> notification/src/main/java/org/apache/atlas/hook/AtlasHook.java 71029b0
> notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 1ee62d2
> notification/src/main/java/org/apache/atlas/notification/NotificationException.java d9d89df
> notification/src/test/java/org/apache/atlas/hook/AtlasHookTest.java 16cb0f0
> notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java 219bd70
>
> Diff: https://reviews.apache.org/r/48723/diff/
>
>
> Testing
> -------
>
> Tested by integrating with hive hook. Brought down Kafka when the hook has to send messages. Ensured that the message is logged into the log file.
>
> Added some UTs for the changes.
>
>
> Thanks,
>
> Hemanth Yamijala
>
>
Re: Review Request 48723: ATLAS-901 Log messages that cannot be sent
to Kafka to a specific log configuration
Posted by Hemanth Yamijala <yh...@gmail.com>.
> On June 16, 2016, 6:58 a.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java, line 57
> > <https://reviews.apache.org/r/48723/diff/1/?file=1419538#file1419538line57>
> >
> > Consider obtaining the logfile name from configuration - similar to flag "atlas.notification.log.failed.messages" in AtlasHook.java.
> >
> > This will enable users to customize the log file location per deployment needs. The default log file can be in directory System.getProperty("atlas.log.dir").
>
> Madhan Neethiraj wrote:
> Since this runs the host component process, "atlas.log.dir" would not be set. Please ignore this suggestion; instead we can enable failed messages logging only if the log file location is specified in the configuration - in a property named like "atlas.hook.notification.failed.messages.filename"
>
> Hemanth Yamijala wrote:
> I have made the log file name configurable. However, a slight change to your proposal: I did retain the boolean to check if the logging is necessary or not. My rationale was mainly to have a default value of true for logging failed messages in the interest of increased resilience. Having a log file with no default value would have prevented this. And having a default value for the file would have meant it is logged always (if we use the presence of the value as an indicator for logging). Please let me know if you feel strongly about having just one configuration. I will leave the issue open to hear back from you.
Another thing is that it is probably easier for admins to turn on / off logging and retain the same file name across runs without having to remember. This way it is easier for tools built to read these messages and replay them to not have to worry about multiple file names that the user could configure etc.
- Hemanth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48723/#review137927
-----------------------------------------------------------
On June 16, 2016, 12:21 p.m., Hemanth Yamijala wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48723/
> -----------------------------------------------------------
>
> (Updated June 16, 2016, 12:21 p.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-901
> https://issues.apache.org/jira/browse/ATLAS-901
>
>
> Repository: atlas
>
>
> Description
> -------
>
> The description of the approach is documented on JIRA here: https://issues.apache.org/jira/browse/ATLAS-901?focusedCommentId=15331480&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15331480
>
>
> Diffs
> -----
>
> docs/src/site/twiki/Configuration.twiki 0e122fe
> notification/src/main/java/org/apache/atlas/hook/AtlasHook.java 71029b0
> notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 1ee62d2
> notification/src/main/java/org/apache/atlas/notification/NotificationException.java d9d89df
> notification/src/test/java/org/apache/atlas/hook/AtlasHookTest.java 16cb0f0
> notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java 219bd70
>
> Diff: https://reviews.apache.org/r/48723/diff/
>
>
> Testing
> -------
>
> Tested by integrating with hive hook. Brought down Kafka when the hook has to send messages. Ensured that the message is logged into the log file.
>
> Added some UTs for the changes.
>
>
> Thanks,
>
> Hemanth Yamijala
>
>
Re: Review Request 48723: ATLAS-901 Log messages that cannot be sent
to Kafka to a specific log configuration
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48723/#review137927
-----------------------------------------------------------
notification/src/main/java/org/apache/atlas/hook/AtlasHook.java (line 123)
<https://reviews.apache.org/r/48723/#comment203153>
if 'messages' contains multiple messages, notification processing seems to stop at the first failure - in KafkaNotification.java. Logging here only writes the failed message; subsequent message in the list will not be logged. Consider NotificationException to store list of messages not sent; and update the logging here to write all these messages.
notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java (line 57)
<https://reviews.apache.org/r/48723/#comment203152>
Consider obtaining the logfile name from configuration - similar to flag "atlas.notification.log.failed.messages" in AtlasHook.java.
This will enable users to customize the log file location per deployment needs. The default log file can be in directory System.getProperty("atlas.log.dir").
- Madhan Neethiraj
On June 15, 2016, 10:11 a.m., Hemanth Yamijala wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48723/
> -----------------------------------------------------------
>
> (Updated June 15, 2016, 10:11 a.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-901
> https://issues.apache.org/jira/browse/ATLAS-901
>
>
> Repository: atlas
>
>
> Description
> -------
>
> The description of the approach is documented on JIRA here: https://issues.apache.org/jira/browse/ATLAS-901?focusedCommentId=15331480&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15331480
>
>
> Diffs
> -----
>
> notification/src/main/java/org/apache/atlas/hook/AtlasHook.java 71029b0
> notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 1ee62d2
> notification/src/main/java/org/apache/atlas/notification/NotificationException.java d9d89df
> notification/src/test/java/org/apache/atlas/hook/AtlasHookTest.java 16cb0f0
>
> Diff: https://reviews.apache.org/r/48723/diff/
>
>
> Testing
> -------
>
> Tested by integrating with hive hook. Brought down Kafka when the hook has to send messages. Ensured that the message is logged into the log file.
>
> Added some UTs for the changes.
>
>
> Thanks,
>
> Hemanth Yamijala
>
>
Re: Review Request 48723: ATLAS-901 Log messages that cannot be sent
to Kafka to a specific log configuration
Posted by Hemanth Yamijala <yh...@gmail.com>.
> On June 16, 2016, 2:53 p.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/hook/AtlasHook.java, line 128
> > <https://reviews.apache.org/r/48723/diff/2/?file=1421382#file1421382line128>
> >
> > Retry attempts should send only failed messages - to avoid sending messages multiple times. Please review.
The sending of the messages is in the else part of the logic - when the retries are completed and we are ready to bail out. So, they do not send multiple times.
> On June 16, 2016, 2:53 p.m., Madhan Neethiraj wrote:
> > notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java, line 75
> > <https://reviews.apache.org/r/48723/diff/2/?file=1421383#file1421383line75>
> >
> > Would the log file location be deterministic - if there are multiple file appenders with different log file directories (for example host component's audit appender could write to a differnt location from log file location)?
> >
> > Wouldn't it be simpler to get fully qualified filename in atlas.notification.failed.messages.filename configuration?
I am getting the configured appender for the root logger specifically. Also I take the first one in the list and break out. Would this not be only one?
- Hemanth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48723/#review137984
-----------------------------------------------------------
On June 16, 2016, 12:21 p.m., Hemanth Yamijala wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48723/
> -----------------------------------------------------------
>
> (Updated June 16, 2016, 12:21 p.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-901
> https://issues.apache.org/jira/browse/ATLAS-901
>
>
> Repository: atlas
>
>
> Description
> -------
>
> The description of the approach is documented on JIRA here: https://issues.apache.org/jira/browse/ATLAS-901?focusedCommentId=15331480&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15331480
>
>
> Diffs
> -----
>
> docs/src/site/twiki/Configuration.twiki 0e122fe
> notification/src/main/java/org/apache/atlas/hook/AtlasHook.java 71029b0
> notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 1ee62d2
> notification/src/main/java/org/apache/atlas/notification/NotificationException.java d9d89df
> notification/src/test/java/org/apache/atlas/hook/AtlasHookTest.java 16cb0f0
> notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java 219bd70
>
> Diff: https://reviews.apache.org/r/48723/diff/
>
>
> Testing
> -------
>
> Tested by integrating with hive hook. Brought down Kafka when the hook has to send messages. Ensured that the message is logged into the log file.
>
> Added some UTs for the changes.
>
>
> Thanks,
>
> Hemanth Yamijala
>
>
Re: Review Request 48723: ATLAS-901 Log messages that cannot be sent
to Kafka to a specific log configuration
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48723/#review137984
-----------------------------------------------------------
notification/src/main/java/org/apache/atlas/hook/AtlasHook.java (line 128)
<https://reviews.apache.org/r/48723/#comment203193>
Retry attempts should send only failed messages - to avoid sending messages multiple times. Please review.
notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java (line 75)
<https://reviews.apache.org/r/48723/#comment203191>
Would the log file location be deterministic - if there are multiple file appenders with different log file directories (for example host component's audit appender could write to a differnt location from log file location)?
Wouldn't it be simpler to get fully qualified filename in atlas.notification.failed.messages.filename configuration?
- Madhan Neethiraj
On June 16, 2016, 12:21 p.m., Hemanth Yamijala wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48723/
> -----------------------------------------------------------
>
> (Updated June 16, 2016, 12:21 p.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-901
> https://issues.apache.org/jira/browse/ATLAS-901
>
>
> Repository: atlas
>
>
> Description
> -------
>
> The description of the approach is documented on JIRA here: https://issues.apache.org/jira/browse/ATLAS-901?focusedCommentId=15331480&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15331480
>
>
> Diffs
> -----
>
> docs/src/site/twiki/Configuration.twiki 0e122fe
> notification/src/main/java/org/apache/atlas/hook/AtlasHook.java 71029b0
> notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 1ee62d2
> notification/src/main/java/org/apache/atlas/notification/NotificationException.java d9d89df
> notification/src/test/java/org/apache/atlas/hook/AtlasHookTest.java 16cb0f0
> notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java 219bd70
>
> Diff: https://reviews.apache.org/r/48723/diff/
>
>
> Testing
> -------
>
> Tested by integrating with hive hook. Brought down Kafka when the hook has to send messages. Ensured that the message is logged into the log file.
>
> Added some UTs for the changes.
>
>
> Thanks,
>
> Hemanth Yamijala
>
>
Re: Review Request 48723: ATLAS-901 Log messages that cannot be sent
to Kafka to a specific log configuration
Posted by Madhan Neethiraj <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48723/#review137997
-----------------------------------------------------------
Ship it!
Ship It!
- Madhan Neethiraj
On June 16, 2016, 12:21 p.m., Hemanth Yamijala wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48723/
> -----------------------------------------------------------
>
> (Updated June 16, 2016, 12:21 p.m.)
>
>
> Review request for atlas.
>
>
> Bugs: ATLAS-901
> https://issues.apache.org/jira/browse/ATLAS-901
>
>
> Repository: atlas
>
>
> Description
> -------
>
> The description of the approach is documented on JIRA here: https://issues.apache.org/jira/browse/ATLAS-901?focusedCommentId=15331480&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15331480
>
>
> Diffs
> -----
>
> docs/src/site/twiki/Configuration.twiki 0e122fe
> notification/src/main/java/org/apache/atlas/hook/AtlasHook.java 71029b0
> notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java PRE-CREATION
> notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 1ee62d2
> notification/src/main/java/org/apache/atlas/notification/NotificationException.java d9d89df
> notification/src/test/java/org/apache/atlas/hook/AtlasHookTest.java 16cb0f0
> notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java 219bd70
>
> Diff: https://reviews.apache.org/r/48723/diff/
>
>
> Testing
> -------
>
> Tested by integrating with hive hook. Brought down Kafka when the hook has to send messages. Ensured that the message is logged into the log file.
>
> Added some UTs for the changes.
>
>
> Thanks,
>
> Hemanth Yamijala
>
>
Re: Review Request 48723: ATLAS-901 Log messages that cannot be sent
to Kafka to a specific log configuration
Posted by Hemanth Yamijala <yh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48723/
-----------------------------------------------------------
(Updated June 16, 2016, 12:21 p.m.)
Review request for atlas.
Changes
-------
Addressed review comments from Madhan.
Bugs: ATLAS-901
https://issues.apache.org/jira/browse/ATLAS-901
Repository: atlas
Description
-------
The description of the approach is documented on JIRA here: https://issues.apache.org/jira/browse/ATLAS-901?focusedCommentId=15331480&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15331480
Diffs (updated)
-----
docs/src/site/twiki/Configuration.twiki 0e122fe
notification/src/main/java/org/apache/atlas/hook/AtlasHook.java 71029b0
notification/src/main/java/org/apache/atlas/hook/FailedMessagesLogger.java PRE-CREATION
notification/src/main/java/org/apache/atlas/kafka/KafkaNotification.java 1ee62d2
notification/src/main/java/org/apache/atlas/notification/NotificationException.java d9d89df
notification/src/test/java/org/apache/atlas/hook/AtlasHookTest.java 16cb0f0
notification/src/test/java/org/apache/atlas/kafka/KafkaNotificationTest.java 219bd70
Diff: https://reviews.apache.org/r/48723/diff/
Testing
-------
Tested by integrating with hive hook. Brought down Kafka when the hook has to send messages. Ensured that the message is logged into the log file.
Added some UTs for the changes.
Thanks,
Hemanth Yamijala