----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33203/#review80299 -----------------------------------------------------------
Bosco - I haven't completed reviewing yet. Here are the comments so far. I will need couple of more hours to complete the review; will do that by tomorrow. agents-audit/src/main/java/org/apache/ranger/audit/model/AuthzAuditEvent.java <https://reviews.apache.org/r/33203/#comment130109> Couldn't eventTime field be used instead of adding field seq_num? agents-audit/src/main/java/org/apache/ranger/audit/model/AuthzAuditEvent.java <https://reviews.apache.org/r/33203/#comment130110> eventCount might be a better name, instead of frequentCount. agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditAsyncQueue.java <https://reviews.apache.org/r/33203/#comment130113> return from log(event) is dropped here. It shoud instead be propagated up. agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditAsyncQueue.java <https://reviews.apache.org/r/33203/#comment130121> Please set consumerThread=null here. agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditAsyncQueue.java <https://reviews.apache.org/r/33203/#comment130116> It might be better to not clear eventList here - in case the consumer keeps a refernece for later use. agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditAsyncQueue.java <https://reviews.apache.org/r/33203/#comment130117> On receiving InterruptedException, shouldn't the while(true) loop terminate? irrespetive of isDrain()/queue.isEmpty()? agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditBatchProcessor.java <https://reviews.apache.org/r/33203/#comment130122> Please set consumerThread=null here. agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditProvider.java <https://reviews.apache.org/r/33203/#comment130128> Given that some customers would have added custom AuditProviders, for example to send Ranger Audit logs to their desired destination, it will be important to keep this interface backward compatible i.e. pretty much no change:-( agents-audit/src/main/java/org/apache/ranger/audit/provider/BaseAuditProvider.java <https://reviews.apache.org/r/33203/#comment130123> BaseAuditProvider has become too complicated (to be called as Base)! fileSpooler, consumer, drain, etc.. It might be cleaner to move all the new functionalty to another new class; and keep BaseAuditProvider simple. agents-audit/src/main/java/org/apache/ranger/audit/provider/BufferedAuditProvider.java <https://reviews.apache.org/r/33203/#comment130129> Shouldn't the return be false if line #53 returns false? agents-audit/src/main/java/org/apache/ranger/audit/provider/BufferedAuditProvider.java <https://reviews.apache.org/r/33203/#comment130130> Shouldn't the return from log(event) be propagated, instead of returning true? agents-audit/src/main/java/org/apache/ranger/audit/provider/BufferedAuditProvider.java <https://reviews.apache.org/r/33203/#comment130131> return false? Or propogate the return from logJSON(event)? agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java <https://reviews.apache.org/r/33203/#comment130132> Shouldn't the return from log(event) be propagated, instead of returning true? agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java <https://reviews.apache.org/r/33203/#comment130133> return false? Or propogate the return from logJSON(event)? - Madhan Neethiraj On April 15, 2015, 1:07 a.m., Don Bosco Durai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33203/ > ----------------------------------------------------------- > > (Updated April 15, 2015, 1:07 a.m.) > > > Review request for ranger, Alok Lal, Madhan Neethiraj, and Selvamohan > Neethiraj. > > > Bugs: RANGER-397 > https://issues.apache.org/jira/browse/RANGER-397 > > > Repository: ranger > > > Description > ------- > > Implement reliable streaming audits to configurable destinations > > > Diffs > ----- > > .gitignore 2c746ed > > agents-audit/src/main/java/org/apache/ranger/audit/model/AuditEventBase.java > 82fcab8 > > agents-audit/src/main/java/org/apache/ranger/audit/model/AuthzAuditEvent.java > d0c1526 > > agents-audit/src/main/java/org/apache/ranger/audit/provider/AsyncAuditProvider.java > 5da5064 > > agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditAsyncQueue.java > PRE-CREATION > > agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditBatchProcessor.java > PRE-CREATION > > agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditDestination.java > PRE-CREATION > > agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditFileSpool.java > PRE-CREATION > > agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditMessageException.java > PRE-CREATION > > agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditProvider.java > 47c2d7f > > agents-audit/src/main/java/org/apache/ranger/audit/provider/AuditProviderFactory.java > bb8fa6d > > agents-audit/src/main/java/org/apache/ranger/audit/provider/BaseAuditProvider.java > a068b8f > > agents-audit/src/main/java/org/apache/ranger/audit/provider/BufferedAuditProvider.java > be32519 > > agents-audit/src/main/java/org/apache/ranger/audit/provider/DbAuditProvider.java > 7414a7a > > agents-audit/src/main/java/org/apache/ranger/audit/provider/DummyAuditProvider.java > a6e3ef1 > > agents-audit/src/main/java/org/apache/ranger/audit/provider/FileAuditDestination.java > PRE-CREATION > > agents-audit/src/main/java/org/apache/ranger/audit/provider/HDFSAuditDestination.java > PRE-CREATION > > agents-audit/src/main/java/org/apache/ranger/audit/provider/LocalFileLogBuffer.java > cdc4d53 > > agents-audit/src/main/java/org/apache/ranger/audit/provider/Log4jAuditProvider.java > 0d0809a > > agents-audit/src/main/java/org/apache/ranger/audit/provider/LogDestination.java > 44e94ad > agents-audit/src/main/java/org/apache/ranger/audit/provider/MiscUtil.java > 17230b2 > > agents-audit/src/main/java/org/apache/ranger/audit/provider/MultiDestAuditProvider.java > 1eec345 > > agents-audit/src/main/java/org/apache/ranger/audit/provider/hdfs/HdfsAuditProvider.java > 620951c > > agents-audit/src/main/java/org/apache/ranger/audit/provider/hdfs/HdfsLogDestination.java > 6b5cb4b > > agents-audit/src/main/java/org/apache/ranger/audit/provider/kafka/KafkaAuditProvider.java > 0ec8790 > > agents-audit/src/main/java/org/apache/ranger/audit/provider/solr/SolrAuditProvider.java > 1b463e6 > security-admin/.gitignore 4a3ed53 > security-admin/pom.xml 3220886 > > security-admin/src/test/java/org/apache/ranger/audit/TestAuditProcessor.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/33203/diff/ > > > Testing > ------- > > Added JUnits to test the features > > > Thanks, > > Don Bosco Durai > >
