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