> On June 17, 2015, 9:03 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java, line 75 > > <https://reviews.apache.org/r/35397/diff/2/?file=985834#file985834line75> > > > > use "if" to be consistent?
I thought there may be some class paths that contain multiple dots, but after some checking not I think it is not possible. So changing it. > On June 17, 2015, 9:03 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java, lines > > 162-163 > > <https://reviews.apache.org/r/35397/diff/2/?file=985834#file985834line162> > > > > doc may not be very precise. It's also possible that this class is > > blacklisted not ClassNotFoundException, right? I guess this is the reason > > you do not put line 164 in line 156 to simplify the code. If the class is blacklisted, we will just avoid loading it in the current classloader, and we should expect it to be loadable in the parent classloader. So I think the comment here is correct. > On June 17, 2015, 9:03 p.m., Yan Fang wrote: > > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, > > lines 426-428 > > <https://reviews.apache.org/r/35397/diff/2/?file=985836#file985836line426> > > > > do we want the TaskClassLoader to take care of the StreamTask itself? I > > thought we only used the TaskClassLoader to take care of what happens > > *inside* the StreamTask. We need the TaskClassLoader to load the user defined XXStreamTask class (not the StreamTask interface class) so that any other classes referenced inside the user defined task will also be auto-loaded by this TaskClassLoader. > On June 17, 2015, 9:03 p.m., Yan Fang wrote: > > samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala, > > lines 103-111 > > <https://reviews.apache.org/r/35397/diff/2/?file=985837#file985837line103> > > > > a little concernted about this. This means we will load the class every > > time a message comes. Is this too much? > > > > My suggestion is to put this code in RunLoop.runs, before the loop > > starts. What do you think? A class will only be loaded once, so I think the overhead should be minimal. My concern for putting this code in RunLoop is that some of the logic in RunLoop like consumerMultiplexer.choose are platform code and any of its referenced classes should not be loaded by the task class loader. - Guozhang ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35397/#review88260 ----------------------------------------------------------- On June 16, 2015, 5:22 p.m., Guozhang Wang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/35397/ > ----------------------------------------------------------- > > (Updated June 16, 2015, 5:22 p.m.) > > > Review request for samza. > > > Bugs: SAMZA-697 > https://issues.apache.org/jira/browse/SAMZA-697 > > > Repository: samza > > > Description > ------- > > Use a separate post-delegate classloader for user-defined tasks > > > Diffs > ----- > > bin/check-all.sh 0725b82ed4f70b155f8bfe65cc6938d4647142d0 > docs/learn/documentation/versioned/jobs/logging.md > 1d13d151316bd51c7a3730e40450000433b7968e > 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 > >
