> On Oct. 28, 2014, 10:06 p.m., Raghav Gautam wrote: > > falcon-regression/merlin-core/src/main/java/org/apache/falcon/regression/Entities/FeedMerlin.java, > > line 198 > > <https://reviews.apache.org/r/27280/diff/1/?file=735308#file735308line198> > > > > This method seems scenario spcific. May be it should be part of > > feedRetryTest(). > > > > Inline perhaps ?
i though this can be used in other test also. will move to RetryTest for now, if in future need arise can move to common util. leaving it seperate and not making inline. > On Oct. 28, 2014, 10:06 p.m., Raghav Gautam wrote: > > falcon-regression/merlin-core/src/main/java/org/apache/falcon/regression/core/util/HadoopUtil.java, > > line 498 > > <https://reviews.apache.org/r/27280/diff/1/?file=735310#file735310line498> > > > > This assumes that test user has privileges to proxy as hdfs. Which is a > > strong assumption. > > > > ACL_OWNER for intents and purposes is current user. Perhaps it would be > > better to setPermission as current user ? > > > > Also, if directory creation and setting permissions is not being done > > frequently, can we keep them as two separate methods. in the current code variable PROXY_USER defined at top as private static final String PROXY_USER = "oozie" ; is trying to act as hdfs. Here assumtion is that user "oozie" has privileges on hdfs. it was done assuming oozie is running as user oozie and will be present in hadoop conf where regression is running. We surely make it configurable if required. permissions are being set as ACL_OWNER authFs.setPermission(path, fsPermission); authFs.setOwner(path, MerlinConstants.ACL_OWNER, PROXY_GROUP); > On Oct. 28, 2014, 10:06 p.m., Raghav Gautam wrote: > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/RetryTest.java, > > line 127 > > <https://reviews.apache.org/r/27280/diff/1/?file=735317#file735317line127> > > > > changing name of setRetry to setProcessRetry would make things much > > clearer. done > On Oct. 28, 2014, 10:06 p.m., Raghav Gautam wrote: > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/RetryTest.java, > > line 285 > > <https://reviews.apache.org/r/27280/diff/1/?file=735317#file735317line285> > > > > javadoc ? done > On Oct. 28, 2014, 10:06 p.m., Raghav Gautam wrote: > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/RetryTest.java, > > line 289 > > <https://reviews.apache.org/r/27280/diff/1/?file=735317#file735317line289> > > > > unit of delayVal ? since this is a test case specific method where we know the limits of delay , i dont think we shuold face any overflow. If that was your concern > On Oct. 28, 2014, 10:06 p.m., Raghav Gautam wrote: > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/RetryTest.java, > > line 295 > > <https://reviews.apache.org/r/27280/diff/1/?file=735317#file735317line295> > > > > may be use switch instead of if ? since only one "if else" is present, i dont think it will make much difference. > On Oct. 28, 2014, 10:06 p.m., Raghav Gautam wrote: > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/RetryTest.java, > > line 297 > > <https://reviews.apache.org/r/27280/diff/1/?file=735317#file735317line297> > > > > Can you please simplify this expression - may be by using temp > > variables ? i have added java doc for the same. that should help. > On Oct. 28, 2014, 10:06 p.m., Raghav Gautam wrote: > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/RetryTest.java, > > line 221 > > <https://reviews.apache.org/r/27280/diff/1/?file=735317#file735317line221> > > > > formatting ? i am not getting any new checkstyle errors. Should i be checking something more ? - samarth ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27280/#review58882 ----------------------------------------------------------- On Oct. 28, 2014, 1:15 p.m., samarth gupta wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27280/ > ----------------------------------------------------------- > > (Updated Oct. 28, 2014, 1:15 p.m.) > > > Review request for Falcon. > > > Repository: falcon-git > > > Description > ------- > > https://issues.apache.org/jira/browse/FALCON-618 > > > Diffs > ----- > > > falcon-regression/merlin-core/src/main/java/org/apache/falcon/regression/Entities/FeedMerlin.java > 02f572e > > falcon-regression/merlin-core/src/main/java/org/apache/falcon/regression/core/enumsAndConstants/MerlinConstants.java > dab5d2c > > falcon-regression/merlin-core/src/main/java/org/apache/falcon/regression/core/util/HadoopUtil.java > 024a652 > > falcon-regression/merlin-core/src/main/java/org/apache/falcon/regression/core/util/OSUtil.java > 86d4d47 > > falcon-regression/merlin-core/src/main/java/org/apache/falcon/regression/core/util/OozieUtil.java > c6217c1 > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/NewRetryTest.java > d4f31e9 > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/ProcessInstanceResumeTest.java > f564bb4 > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/ProcessInstanceStatusTest.java > a94cf1a > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/ProcessLateRerunTest.java > 3f7258e > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/RetryTest.java > PRE-CREATION > > falcon-regression/merlin/src/test/java/org/apache/falcon/regression/prism/PrismFeedUpdateTest.java > 9b5d770 > > falcon-regression/merlin/src/test/resources/RetryTests/valid1/bundle1/process-agg.xml > 03cb727 > > Diff: https://reviews.apache.org/r/27280/diff/ > > > Testing > ------- > > > Thanks, > > samarth gupta > >
