----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50098/#review142521 -----------------------------------------------------------
agents-audit/src/main/java/org/apache/ranger/audit/destination/HDFSAuditDestination.java (line 114) <https://reviews.apache.org/r/50098/#comment208086> // when rolloverPeriod is not specified, use fileRolloverSec if(StringUtils.isEmpty(rolloverPeriod)) { rolloverPeriod = rollingTimeUtil.convertRolloverSecondsToRolloverPeriod(fileRolloverSec); } agents-audit/src/main/java/org/apache/ranger/audit/destination/HDFSAuditDestination.java (line 122) <https://reviews.apache.org/r/50098/#comment208088> Consider printing the exception details in the warn log - by adding the exception to the argument list. agents-audit/src/main/java/org/apache/ranger/audit/destination/HDFSAuditDestination.java (line 327) <https://reviews.apache.org/r/50098/#comment208087> Consider printing the exception details in the warn log - by adding the exception to the argument list. agents-audit/src/main/java/org/apache/ranger/audit/utils/RollingTimeUtil.java (line 60) <https://reviews.apache.org/r/50098/#comment208083> Consider passing the 'computePeriod' as argument to getTimeNumeral(), as it is already computed in the previous line. agents-audit/src/main/java/org/apache/ranger/audit/utils/RollingTimeUtil.java (line 82) <https://reviews.apache.org/r/50098/#comment208080> Consider handling empty/null value for rollingTimePeriod in getTimeLiteral() - along withe other validation of the value. agents-audit/src/main/java/org/apache/ranger/audit/utils/RollingTimeUtil.java (line 99) <https://reviews.apache.org/r/50098/#comment208084> This method seems to convert value of "file.rollover.sec" to equivalent "file.rollover.period" - when possible. In that case, consider this method to just do that. Also, I think it will be safer for this method to deal with only days/hours/minutes granularity. public Date convertRolloverSecondsToRolloverPeriod(long duration) { final int SECONDS_IN_MINUTE = 60; final int SECONDS_IN_HOUR = 60 * SECONDS_IN_MINUTE; final int SECONDS_IN_DAY = 24 * SECONDS_IN_HOUR; String ret = null; int days = duration / SECONDS_IN_DAY; duration %= SECONDS_IN_DAY; int hours = duration / SECONDS_IN_HOUR; duration %= SECONDS_IN_HOUR; int minutes = duration / SECONDS_IN_MINUTE; if(days != 0) { if(hours == 0 && minutes == 0) { ret = (days + DAYS); } } else if(hours != 0) { if(minutes == 0) { ret = (hours + HOURS); } } else if(minutes != 0) { ret = (minutes + MINUTES); } return ret; } public Date computeNextRollingTime(long durationSeconds, Date previousRolloverTime) { long now = System.currentTimeMillis(); long nextRolloverTime = (previousRolloverTime == null) ? now : previousRolloverTime.getTime(); long durationMillis = (durationSeconds < 1 ? 1 : durationSeconds) * 1000; while(nextRolloverTime <= now) { nextRolloverTime += durationMillis; } return nextRolloverTime; } agents-audit/src/main/java/org/apache/ranger/audit/utils/RollingTimeUtil.java (line 139) <https://reviews.apache.org/r/50098/#comment208085> Shouldn't line #139 be: calendarMonth.set(Calendar.DAY_OF_MONTH, 1); agents-audit/src/main/java/org/apache/ranger/audit/utils/RollingTimeUtil.java (line 223) <https://reviews.apache.org/r/50098/#comment208082> Consider following implementation: if(StringUtils.isEmpty(rollOverPeriod) { throw new Exception("empty rollover period") } else if(rollOverPeriod.endsWith(MINUTES)) { ret = MINUTES; } else if(rollOverPeriod.endsWith(HOURS)) { ret = HOURS; } else if(rollOverPeriod.endsWith(DAYS)) { ret = DAYS; } else if(rollOverPeriod.endsWith(WEEKS)) { ret = WEEKS; } else if(rollOverPeriod.endsWith(MONTHS)) { ret = MONTHS; } else if(rollOverPeriod.endsWith(YEARS)) { ret = YEARS; } else { throw new Exception(rollOverPeriod + ": invalid rollover period") } return ret; - Madhan Neethiraj On July 16, 2016, 12:15 a.m., Ramesh Mani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50098/ > ----------------------------------------------------------- > > (Updated July 16, 2016, 12:15 a.m.) > > > Review request for ranger, Madhan Neethiraj, Selvamohan Neethiraj, and > Velmurugan Periasamy. > > > Repository: ranger > > > Description > ------- > > RANGER-1105: Ranger should provide configuration to have do hdfs audit file > rollover at absolute time > > > Diffs > ----- > > > agents-audit/src/main/java/org/apache/ranger/audit/destination/HDFSAuditDestination.java > 519d943 > > agents-audit/src/main/java/org/apache/ranger/audit/utils/RollingTimeUtil.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/50098/diff/ > > > Testing > ------- > > Testing done in local VM for hdfs file rollover. > > > Thanks, > > Ramesh Mani > >
