----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35397/#review88455 -----------------------------------------------------------
This is partial review (I didn't go thru the test). samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 50) <https://reviews.apache.org/r/35397/#comment140992> Do we have test for this case? samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 63) <https://reviews.apache.org/r/35397/#comment140989> Do we need a LOG.warn in case the file doesn't exist. samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 72) <https://reviews.apache.org/r/35397/#comment140993> nit. may be if (blacklistClassnames == null) return false. samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 103) <https://reviews.apache.org/r/35397/#comment140995> should we have little bit more validation here (checking for empty strings for example) samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 109) <https://reviews.apache.org/r/35397/#comment140996> nit. should be checked at the beginning of the method. samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 129) <https://reviews.apache.org/r/35397/#comment140997> nit. 'blacklisted' samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 140) <https://reviews.apache.org/r/35397/#comment140998> else log an error? samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 146) <https://reviews.apache.org/r/35397/#comment141000> do we need to override it if it is the same a default implementation. samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 435) <https://reviews.apache.org/r/35397/#comment141001> Do we want to create this taskClassLoader if the taskClassLoaderPath is not configured? If this is the case we are creating classLoader with 'null' list of URLs. Is it safe? This - Boris Shkolnik On June 18, 2015, 6:42 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35397/ > ----------------------------------------------------------- > > (Updated June 18, 2015, 6:42 p.m.) > > > Review request for samza. > > > Bugs: SAMZA-697 > https://issues.apache.org/jira/browse/SAMZA-697 > > > Repository: samza > > > Description > ------- > > Address Yan's comments > > > Diffs > ----- > > checkstyle/import-control.xml 3374f0c432e61ac4cda275377604cfd481f0cddf > docs/learn/documentation/versioned/jobs/configuration-table.html > 405e2cea4fd1d037cc26b3537f6bb406eded202b > samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala > 0b3a235b5ab1d6bd60669bfe6023f6b0b4e943d3 > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > cbacd183420e9d1d72b05693b55a8f0a62d59fc5 > samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala > c5a5ea5dea9a950fc741625238f5bf8b1f362180 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 1c178a661e449c6bdfc4ce431aef9bb2d261a6c2 > > samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala > 4fac154709d72ab594485dad93c912b55fb1617e > samza-core/src/test/java/org/apache/samza/task/TestTaskClassLoader.java > PRE-CREATION > > samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala > 9fb1aa98fcd14397e8a4cb00c67537482e95fa53 > samza-core/src/test/scala/org/apache/samza/container/TestTaskInstance.scala > 7caad28c9298485753ab861da76793cf925953ed > > Diff: https://reviews.apache.org/r/35397/diff/ > > > Testing > ------- > > unit tests > > > Thanks, > > Guozhang Wang > >
