samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>
Subject Re: Review Request 35397: Fix SAMZA-697
Date Fri, 19 Jun 2015 08:59:47 GMT

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


Overall looks good. I have a few comments/questions. Thanks!


bin/check-all.sh (line 60)
<https://reviews.apache.org/r/35397/#comment140653>

    I think that this is from another commit? You may need to rebase your changes.



docs/learn/documentation/versioned/jobs/configuration-table.html (line 443)
<https://reviews.apache.org/r/35397/#comment141085>

    Users who extends and implements StreamTask usually have their implementation classes
put in the same package as org.apache.samza.task. Wouldn't this default blacklist also ignore
the user-implemented StreamTask classes?



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

    nit: for unit test, changing it to package default should work.



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

    Question: does this findLoadedClass(name) return true if the same binary name of a class
has been loaded by another classloader? Can we add a test here to make sure?



samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 433)
<https://reviews.apache.org/r/35397/#comment141089>

    What if the taskClassLoaderPath is null? Shouldn't we just skip the swaping of classloaders?
    
    Maybe it would be good to print out a warning now and use the default classloader. Then
later enforcing it by throwing exception if taskClassLoaderPath is not setup.



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

    It would seem cleaner as:
    
    def swapClassloader(userTask: => Unit) {
      val thread = Thread.currentThread
      val oldClassLoader = thread.getContextClassLoader
      thread.setContextClassLoader(taskClassLoader)
      try {
        userTask
      } finally {
        thread.setContextClassLoader(oldClassLoader)
      }
    }
    
    Then:
    swapClassloader {
      task.asInstanceOf[InitableTask].init(config, context)
    }
    
    swapClassloader {
      exceptionHandler.maybeHandle {
        task.process(envelope, collector, coordinator)
      }
    }
    ...


- Yi Pan (Data Infrastructure)


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


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