samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jake Maes <jma...@apache.org>
Subject Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
Date Mon, 10 Oct 2016 19:27:28 GMT

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



Initial scan. Will need to take another pass afther the below is addressed.


samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (lines 27 - 33)
<https://reviews.apache.org/r/52492/#comment220742>

    Based on these dependencies, this class is not a generic LocalStoreMonitor. 
    
    It should be made generic. If that's not possible, then it should be at least renamed
to YarnLocalStoreMonitor.



samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (lines 47 - 52)
<https://reviews.apache.org/r/52492/#comment220737>

    https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
    "So when should you use static import? Very sparingly!"



samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 84)
<https://reviews.apache.org/r/52492/#comment220743>

    Shouldn't this be checked only once in the constructor?



samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 114)
<https://reviews.apache.org/r/52492/#comment220750>

    This is not an accurate name. Jobs can be stateful and not have any on disk state. Jobs
can be stateful and not have any state in the logged stores directory. 
    
    I'd recommend using similar terminology as is used in TaskStorageManager. It's accurate
and we should use consistent terminology anyway.



samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 164)
<https://reviews.apache.org/r/52492/#comment220758>

    I think this class is mixing 2 concerns and it would be better to separate them. 
    1. How to determine the urls to query.
    2. Expose a simple client to fetch info from the samza rest API at those urls



samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 166)
<https://reviews.apache.org/r/52492/#comment220741>

    This probably belongs in TasksResource or a TasksResourceConstants class. Declaring it
here makes this class fragile to changes/evolutions in the Tasks API.



samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java (line 170)
<https://reviews.apache.org/r/52492/#comment220740>

    This probably belongs in JobsResource or a JobsResourceConstants class. Declaring it here
makes this class fragile to changes/evolutions in the Jobs API.



samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java (line 23)
<https://reviews.apache.org/r/52492/#comment220738>

    https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
    "So when should you use static import? Very sparingly!"



samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java (line 30)
<https://reviews.apache.org/r/52492/#comment220735>

    This class is introduced in https://reviews.apache.org/r/52168
    
    Why is it duplicated here?



samza-rest/src/main/java/org/apache/samza/rest/model/Task.java (line 30)
<https://reviews.apache.org/r/52492/#comment220734>

    This class is introduced in https://reviews.apache.org/r/52168
    
    Why is it duplicated here?



samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java (line 38)
<https://reviews.apache.org/r/52492/#comment220739>

    https://docs.oracle.com/javase/8/docs/technotes/guides/language/static-import.html
    "So when should you use static import? Very sparingly!"


- Jake Maes


On Oct. 8, 2016, 12:07 a.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 8, 2016, 12:07 a.m.)
> 
> 
> Review request for samza.
> 
> 
> Repository: samza
> 
> 
> Description
> -------
> 
> This patch contains the samza-rest monitor that periodically cleans up the stale local
stores of dead jobs/tasks. It performs the store deletion in two phases. Initially it deletes
the offset file in the local task stores if the following condition is true. ((jobIsNotRunning
|| preferedHost != nmHost) && offsetFilelastModifiedTime is greater than deleteRetention).
During the subsequent run, it deletes the local task stores if it does not contain offset
file. Please refer to the design doc of SAMZA-656 (https://issues.apache.org/jira/secure/attachment/12828083/DESIGN-SAMZA-656.pdf)
for more details.
> 
> 
> Diffs
> -----
> 
>   build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1 
>   samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java PRE-CREATION

>   samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java PRE-CREATION

>   samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorFactory.java PRE-CREATION

>   samza-rest/src/main/java/org/apache/samza/rest/model/Partition.java PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/rest/model/Task.java PRE-CREATION 
>   samza-rest/src/test/java/org/apache/samza/monitor/TestLocalStoreMonitor.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/52492/diff/
> 
> 
> Testing
> -------
> 
> Unit testing and manual testing are done to verify the functionality.
> 
> 
> Thanks,
> 
> Shanthoosh Venkataraman
> 
>


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