samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yan Fang" <yanfang...@gmail.com>
Subject Re: Review Request 35397: Fix SAMZA-697
Date Wed, 17 Jun 2015 21:03:09 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35397/#review88260
-----------------------------------------------------------



samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 44)
<https://reviews.apache.org/r/35397/#comment140682>

    change the scope?



samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 74)
<https://reviews.apache.org/r/35397/#comment140715>

    also canonicalize the classNames in the  blacklistClassnames list?



samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 75)
<https://reviews.apache.org/r/35397/#comment140684>

    use "if" to be consistent?



samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 115)
<https://reviews.apache.org/r/35397/#comment140685>

    to be "if"? Just my $0.02



samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 156)
<https://reviews.apache.org/r/35397/#comment140704>

    log here



samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (lines 162 - 163)
<https://reviews.apache.org/r/35397/#comment140705>

    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.



samza-core/src/main/java/org/apache/samza/task/TaskClassLoader.java (line 164)
<https://reviews.apache.org/r/35397/#comment140706>

    you mean, parent.loadClass(name), right?



samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala (lines 40 - 42)
<https://reviews.apache.org/r/35397/#comment140717>

    add those to configuration table as well, including what kind of format we accept.



samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala (line 112)
<https://reviews.apache.org/r/35397/#comment140707>

    remove the space



samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (lines 425 - 427)
<https://reviews.apache.org/r/35397/#comment140708>

    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.



samza-core/src/main/scala/org/apache/samza/container/TaskInstance.scala (lines 97 - 105)
<https://reviews.apache.org/r/35397/#comment140709>

    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?



samza-core/src/test/java/org/apache/samza/task/TestTaskClassLoader.java (line 88)
<https://reviews.apache.org/r/35397/#comment140716>

    also want to test "org/example/foo" format


- Yan Fang


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message