samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shanthoosh Venkataraman <santhoshvenkat1...@gmail.com>
Subject Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.
Date Tue, 11 Oct 2016 00:53:40 GMT


> On Oct. 10, 2016, 7:27 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java, line 84
> > <https://reviews.apache.org/r/52492/diff/2/?file=1527756#file1527756line84>
> >
> >     Shouldn't this be checked only once in the constructor?

Current monitor loading implementation doesn't load monitors that throws exceptions during
instantiation. If we throw up in the constructor, the monitor won't even load and user may
not be aware of the problem. If we don't validate in constructor, there's a good chance that
the user might notice the problem seeing lot of exceptions getting thrown from monitor method.


> On Oct. 10, 2016, 7:27 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java, line 164
> > <https://reviews.apache.org/r/52492/diff/2/?file=1527756#file1527756line164>
> >
> >     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

Moved all the url constants to query into ResourceConstants class.


- Shanthoosh


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


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