You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@samza.apache.org by Shanthoosh Venkataraman <sa...@gmail.com> on 2016/10/04 22:03:03 UTC

Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

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

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

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Shanthoosh Venkataraman <sa...@gmail.com>.


> On Oct. 17, 2016, 11:22 p.m., Jake Maes wrote:
> > samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java, line 107
> > <https://reviews.apache.org/r/52492/diff/3-4/?file=1530145#file1530145line107>
> >
> >     nit: I think this would read better if demorgans law were applied

Done.


- Shanthoosh


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


On Oct. 20, 2016, 9:52 p.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 20, 2016, 9:52 p.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/resources/ResourceConstants.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Jake Maes <jm...@apache.org>.

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




samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (line 98)
<https://reviews.apache.org/r/52492/#comment222185>

    nit: I think this would read better if demorgans law were applied



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (line 99)
<https://reviews.apache.org/r/52492/#comment222186>

    I see tests for the other 2 conditions, but not the preferredHost



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (lines 164 - 166)
<https://reviews.apache.org/r/52492/#comment222187>

    This class is still using YARN terminology when it is actually generic. Instead of RM hosts, we just want the hosts which support the Jobs and Tasks endpoints. 
    
    JOB_STATUS_SERVERS, perhaps?



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorConfig.java (line 49)
<https://reviews.apache.org/r/52492/#comment222193>

    Same in this class, do we really need to make this Yarn-specific or can we make it generic now?


- Jake Maes


On Oct. 13, 2016, 9:01 p.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 13, 2016, 9:01 p.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/YarnLocalStoreMonitor.java PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorConfig.java PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorFactory.java PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION 
>   samza-rest/src/test/java/org/apache/samza/monitor/TestYarnLocalStoreMonitor.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Jake Maes <jm...@apache.org>.

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


Fix it, then Ship it!




~15 documentation references to YARN should be reworded. Other than that, it looks good. Will commit after https://reviews.apache.org/r/52168/


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

    All references to Yarn should be removed. 
    
    This could be just "node"


- Jake Maes


On Oct. 20, 2016, 9:52 p.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 20, 2016, 9:52 p.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/resources/ResourceConstants.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Boris Shkolnik <bo...@apache.org>.

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




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

    nit. It is beeter to be more precise what the exception is thrown.



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

    I am not sure if we need guava for anything else, but is it worth bringing it in for this check ? IMHO, a regulare if() throw Exception, should suffice.



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

    nit. should we check the return value here?



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

    nit. I think we usually use name LOG.



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

    '.'->':'



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

    InetAddress.getLocalHost().getHostName() should be run outside of the loops.



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

    this should be INFO level.



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

    'an' -> 'this'



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

    please add comment on the format of the name.



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

    nit. Do you need to handle any exceptions here?



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

    Seems like this class may be usefull elsewhere. Is it worth moving it to a top-level class?



samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java (line 40)
<https://reviews.apache.org/r/52492/#comment223697>

    can we add a check for it and give a warning?


- Boris Shkolnik


On Oct. 26, 2016, 12:41 a.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2016, 12:41 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/resources/ResourceConstants.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Boris Shkolnik <bo...@apache.org>.

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


Ship it!





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

    nit. just for readability please move 'InetAddress.getLocalHost().getHostName()' to before the 'if'


- Boris Shkolnik


On Oct. 31, 2016, 11:04 p.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2016, 11:04 p.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/JobsClient.java PRE-CREATION 
>   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/resources/ResourceConstants.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Shanthoosh Venkataraman <sa...@gmail.com>.

(Updated Oct. 31, 2016, 11:04 p.m.)

Review request for samza.

Repository: samza

Description
-------

Diffs (updated)
-----

build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1
samza-rest/src/main/java/org/apache/samza/monitor/JobsClient.java PRE-CREATION
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/resources/ResourceConstants.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

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Shanthoosh Venkataraman <sa...@gmail.com>.

(Updated Oct. 26, 2016, 12:41 a.m.)

Review request for samza.

Repository: samza

Description
-------

Diffs (updated)
-----

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/resources/ResourceConstants.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

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Jake Maes <jm...@apache.org>.

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


Fix it, then Ship it!





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

    I wasn't specific enough. Not just the word/acronym "Yarn", but all Yarn-related terminology should be removed. No "NodeManager", "ResourceManager" or any other yarn component or concept should be referenced.


- Jake Maes


On Oct. 25, 2016, 8:14 p.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 25, 2016, 8:14 p.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/resources/ResourceConstants.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Jagadish Venkatraman <ja...@gmail.com>.


> On Oct. 25, 2016, 9:31 p.m., Jagadish Venkatraman wrote:
> > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java, line 166
> > <https://reviews.apache.org/r/52492/diff/5/?file=1542707#file1542707line166>
> >
> >     JobsClient seems to be a generic class and not really tied to any LocalStoreMonitor. Furthermore, we pass in an instance of the client to the monitor.
> >     
> >     For example, any other monitor that wants to make retriable requests could potentially use this. Can this be a separate class?
> 
> Shanthoosh Venkataraman wrote:
>     There is no other monitor requiring this currently. It would make sense to move this outside, once such a need arises.

0. `Could` another monitor use this?
1. Does this make sense to have a JobsClient instance in its independent entirety? (as opposed to being inside a LocalStoreMonitor). If a nested class would be useful in some other context, then maybe it should be a top-level class.
2. Is the JobsClient completely hidden outside the `LocalStoreMonitor`?

Let's discuss this offline.


- Jagadish


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


On Oct. 26, 2016, 12:41 a.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2016, 12:41 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/resources/ResourceConstants.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Shanthoosh Venkataraman <sa...@gmail.com>.


> On Oct. 25, 2016, 9:31 p.m., Jagadish Venkatraman wrote:
> > samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitor.java, line 166
> > <https://reviews.apache.org/r/52492/diff/5/?file=1542707#file1542707line166>
> >
> >     JobsClient seems to be a generic class and not really tied to any LocalStoreMonitor. Furthermore, we pass in an instance of the client to the monitor.
> >     
> >     For example, any other monitor that wants to make retriable requests could potentially use this. Can this be a separate class?

There is no other monitor requiring this currently. It would make sense to move this outside, once such a need arises.


- Shanthoosh


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


On Oct. 26, 2016, 12:41 a.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 26, 2016, 12:41 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/resources/ResourceConstants.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Jagadish Venkatraman <ja...@gmail.com>.

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




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

    IMO, this can be made info.



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

    Print what the config file TTL is. Also log what the last modified timestamp of the offset file is.



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

    JobsClient seems to be a generic class and not really tied to any LocalStoreMonitor. Furthermore, we pass in an instance of the client to the monitor.
    
    For example, any other monitor that wants to make retriable requests could potentially use this. Can this be a separate class?



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

    Do we update this list? If not, do you think it can be made final? Immutability is a nice property to have in general.



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

    seems that this is just throwing a SamzaException.



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

    Where is the exception being thrown? Is this declared with `throws`?



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

    nit: Docs should accurate the type of exception being thrown. 
    
    Would only RM unavailable throw this exception? It seems to me that any error in reading from stream will also throw it?



samza-rest/src/main/java/org/apache/samza/monitor/LocalStoreMonitorConfig.java (line 49)
<https://reviews.apache.org/r/52492/#comment223189>

    Are these acceseed outside? If so, can't these just be private?
    
    The same applies to all above variables.



samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java (line 23)
<https://reviews.apache.org/r/52492/#comment223188>

    Why do we need this class to store 2 global constants? Are these used elsewhere?
    
    They only seem to be used inside jobs client? It seems that they can be made private variables of that class?


- Jagadish Venkatraman


On Oct. 25, 2016, 8:14 p.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 25, 2016, 8:14 p.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/resources/ResourceConstants.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Shanthoosh Venkataraman <sa...@gmail.com>.

(Updated Oct. 25, 2016, 8:14 p.m.)

Review request for samza.

Repository: samza

Description
-------

Diffs (updated)
-----

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/resources/ResourceConstants.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

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Shanthoosh Venkataraman <sa...@gmail.com>.

(Updated Oct. 20, 2016, 9:52 p.m.)

Review request for samza.

Repository: samza

Description
-------

Diffs (updated)
-----

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/resources/ResourceConstants.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

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Shanthoosh Venkataraman <sa...@gmail.com>.

(Updated Oct. 13, 2016, 9:01 p.m.)

Review request for samza.

Repository: samza

Description
-------

Diffs (updated)
-----

build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1
samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/monitor/TestYarnLocalStoreMonitor.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

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Jake Maes <jm...@apache.org>.

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



More thorough review this time. Feedback below.

Also, we should add documentation for this monitor, particularly if it is going to be enabled by default. The documentation should clearly explain what the monitor does and how it's related to delete retention in a log compacted changelog.


samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (line 82)
<https://reviews.apache.org/r/52492/#comment221325>

    I see config.getLocalStoreBaseDir called twice in quick succession. It implies that the paths are related.
    
    Suggestion:
    Since getHostAffinityEnabledJobs() creates a file for the baseDir, why not 
    1. create that file before the for-loop
    2. pass that file into getHostAffinityEnabledJobs
    3. use that file as the first argument (parent) to create the jobDir File.



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (lines 85 - 91)
<https://reviews.apache.org/r/52492/#comment221333>

    This code doesn't ensure that it will only delete the store for the appropriate task. This could be a problem if there were asymmetry in the stores across tasks
    
    So the file system looks like:
    
    .../job1-1/myStoreName1/task1
    .../job1-1/myStoreName2/task2
    
    task.getStoreNames().contains("myStoreName1") will be false for task2. 
    task.getStoreNames().contains("myStoreName2") will be false for task1. 
    
    And so they will delete each other's stores. 
    
    This might be a stretch, but blindly deleting all the task dirs under a particular store dir seems like a bug waiting to happen.
    
    It would be useful to have some tests to cover these scenarios.



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (line 94)
<https://reviews.apache.org/r/52492/#comment221342>

    3 issues with this section:
    
    1. The big one: The way this code reads, it will delete the store for any non-running job, completely defeating the purpose of host affinity. After staring at it for a while, I realized the deleteTaskStore() method doesn't always delete. This is confusing and makes the code seem destructive. I'd highly recommend a more accurate name, like "markSweepIfStale()"
    2. Do we want to delete the store for a job with status == STARTING? Feels like a race condition.
    
    see JobStatus.hasBeenStarted()
    3. I didn't see any unit tests covering this logic.



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (line 145)
<https://reviews.apache.org/r/52492/#comment221346>

    Thanks for the careful logging.



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (line 158)
<https://reviews.apache.org/r/52492/#comment221324>

    I'd suggest renaming this to JobsClient for 2 reasons. 
    1. It might be something we refactor out to make it easier for users.
    2. "Util" classes are like misc. buckets. They invite developers to lazily throw methods in that bucket rather than diligently planning them, naming them, and putting them where they belong. This almost always turns into a mess over time.



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (line 179)
<https://reviews.apache.org/r/52492/#comment221349>

    This seems wrong. The samza-rest service should not be using the RM port. This request should have nothing to do with YARN.



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (line 192)
<https://reviews.apache.org/r/52492/#comment221350>

    This seems wrong. The samza-rest service should not be using the RM port. This request should have nothing to do with YARN.



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java (line 206)
<https://reviews.apache.org/r/52492/#comment221351>

    Why don't we just have a config "job status servers" that has a list of host:port to samza-rest servers that can provide job status? 
    
    This would be analogous to kafka's bootstrap.servers config.
    
    Then we wouldn't need any Yarn dependency.



samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorConfig.java (line 46)
<https://reviews.apache.org/r/52492/#comment221352>

    This must not be larger than delete.retention.ms!
    
    Slightly lower is better.
    
    So if we're defaulting delete.retention.ms to 24 hrs, I'd set this to 23-23.5
    
    Javadocs should also clearly warn of this association.


- Jake Maes


On Oct. 11, 2016, 12:53 a.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 11, 2016, 12:53 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/YarnLocalStoreMonitor.java PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorConfig.java PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorFactory.java PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 
>   samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION 
>   samza-rest/src/test/java/org/apache/samza/monitor/TestYarnLocalStoreMonitor.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Shanthoosh Venkataraman <sa...@gmail.com>.

(Updated Oct. 11, 2016, 12:53 a.m.)

Review request for samza.

Repository: samza

Description
-------

Diffs (updated)
-----

build.gradle 2bea27b75288d3103178bc3762b9556f6e69cdd1
samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitor.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorConfig.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorFactory.java PRE-CREATION
samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089
samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION
samza-rest/src/test/java/org/apache/samza/monitor/TestYarnLocalStoreMonitor.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

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Shanthoosh Venkataraman <sa...@gmail.com>.


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

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Jake Maes <jm...@apache.org>.


> 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?
> 
> Shanthoosh Venkataraman wrote:
>     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.

Either way the user needs to see the exception in the log. In one case it's a single exception and in the other it creates so much noise you can't see other problems in the log. I prefer the former.


- Jake


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


On Oct. 11, 2016, 12:53 a.m., Shanthoosh Venkataraman wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52492/
> -----------------------------------------------------------
> 
> (Updated Oct. 11, 2016, 12:53 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/YarnLocalStoreMonitor.java PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorConfig.java PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/monitor/YarnLocalStoreMonitorFactory.java PRE-CREATION 
>   samza-rest/src/main/java/org/apache/samza/rest/resources/JobsResource.java a566db598c284d69ea61af88fdc0851483d5a089 
>   samza-rest/src/main/java/org/apache/samza/rest/resources/ResourceConstants.java PRE-CREATION 
>   samza-rest/src/test/java/org/apache/samza/monitor/TestYarnLocalStoreMonitor.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
> 
>

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Jake Maes <jm...@apache.org>.

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

Re: Review Request 52492: Adding monitor to clean up stale local stores of jobs/tasks.

Posted by Shanthoosh Venkataraman <sa...@gmail.com>.

(Updated Oct. 8, 2016, 12:07 a.m.)

Review request for samza.

Repository: samza

Description
-------

Diffs (updated)
-----

Diff: https://reviews.apache.org/r/52492/diff/

Testing
-------

Unit testing and manual testing are done to verify the functionality.

Thanks,

Shanthoosh Venkataraman