You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/10/01 19:26:53 UTC
Review Request 26232: Implementing update history pruner.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-743
https://issues.apache.org/jira/browse/AURORA-743
Repository: aurora
Description
-------
The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
Diff: https://reviews.apache.org/r/26232/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Oct. 1, 2014, 5:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java, line 161
> > <https://reviews.apache.org/r/26232/diff/1/?file=710137#file710137line161>
> >
> > We also need time-based retention. How about 1 month by default?
Thought about that but was not sure the extra complexity is worth it. Also, don't we want to show update history in the UI regardless of how old it is?
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55088
-----------------------------------------------------------
On Oct. 1, 2014, 5:26 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2014, 5:26 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 1, 2014, 5:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java, line 161
> > <https://reviews.apache.org/r/26232/diff/1/?file=710137#file710137line161>
> >
> > We also need time-based retention. How about 1 month by default?
>
> Maxim Khutornenko wrote:
> Thought about that but was not sure the extra complexity is worth it. Also, don't we want to show update history in the UI regardless of how old it is?
>
> Bill Farner wrote:
> Given that it currently needs to fit in memory, i think not.
>
> For example: if i create a job to test something out, then kill it and never return. Probably shouldn't retain those updates forever.
>
> David McLaughlin wrote:
> Wouldn't the updates in that case be killed by the job garbage collection/cascading deletion of the job key?
If so, that warrants validation in an integration test between TaskHistoryPruner, storage, and DbJobUpdateStore.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55088
-----------------------------------------------------------
On Oct. 1, 2014, 5:26 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2014, 5:26 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 1, 2014, 5:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java, line 161
> > <https://reviews.apache.org/r/26232/diff/1/?file=710137#file710137line161>
> >
> > We also need time-based retention. How about 1 month by default?
>
> Maxim Khutornenko wrote:
> Thought about that but was not sure the extra complexity is worth it. Also, don't we want to show update history in the UI regardless of how old it is?
Given that it currently needs to fit in memory, i think not.
For example: if i create a job to test something out, then kill it and never return. Probably shouldn't retain those updates forever.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55088
-----------------------------------------------------------
On Oct. 1, 2014, 5:26 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2014, 5:26 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Oct. 1, 2014, 5:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java, line 161
> > <https://reviews.apache.org/r/26232/diff/1/?file=710137#file710137line161>
> >
> > We also need time-based retention. How about 1 month by default?
>
> Maxim Khutornenko wrote:
> Thought about that but was not sure the extra complexity is worth it. Also, don't we want to show update history in the UI regardless of how old it is?
>
> Bill Farner wrote:
> Given that it currently needs to fit in memory, i think not.
>
> For example: if i create a job to test something out, then kill it and never return. Probably shouldn't retain those updates forever.
>
> David McLaughlin wrote:
> Wouldn't the updates in that case be killed by the job garbage collection/cascading deletion of the job key?
>
> Bill Farner wrote:
> If so, that warrants validation in an integration test between TaskHistoryPruner, storage, and DbJobUpdateStore.
We don't cascade deletes into the job_keys table. In fact, we don't actually delete from job_keys at all at this point. We should probably address this when converting TaskStore into H2.
| if i create a job to test something out, then kill it and never return. Probably shouldn't retain those updates forever.
Good point. This alone warrants adding time-based cleanup.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55088
-----------------------------------------------------------
On Oct. 1, 2014, 5:26 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2014, 5:26 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by David McLaughlin <da...@dmclaughlin.com>.
> On Oct. 1, 2014, 5:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java, line 161
> > <https://reviews.apache.org/r/26232/diff/1/?file=710137#file710137line161>
> >
> > We also need time-based retention. How about 1 month by default?
>
> Maxim Khutornenko wrote:
> Thought about that but was not sure the extra complexity is worth it. Also, don't we want to show update history in the UI regardless of how old it is?
>
> Bill Farner wrote:
> Given that it currently needs to fit in memory, i think not.
>
> For example: if i create a job to test something out, then kill it and never return. Probably shouldn't retain those updates forever.
Wouldn't the updates in that case be killed by the job garbage collection/cascading deletion of the job key?
- David
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55088
-----------------------------------------------------------
On Oct. 1, 2014, 5:26 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2014, 5:26 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Oct. 1, 2014, 5:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java, line 140
> > <https://reviews.apache.org/r/26232/diff/1/?file=710141#file710141line140>
> >
> > I'm not sure how i feel about this. On one hand, it's nice that SQL handles a lot of the complexity. On the other, it would be _really_ nice to see deleted job update UUIDs in the log. Perhaps this function can return some details about the deleted updates?
I have refactored to return a set of IDs and log them in the pruner.
> On Oct. 1, 2014, 5:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java, line 63
> > <https://reviews.apache.org/r/26232/diff/1/?file=710139#file710139line63>
> >
> > Nit: starting this with "Pruning completed" breaks my brain a little. How about "Pruning job update history."
Reworded.
> On Oct. 1, 2014, 5:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java, line 43
> > <https://reviews.apache.org/r/26232/diff/1/?file=710139#file710139line43>
> >
> > maxUpdatesPerJob?
Renamed.
> On Oct. 1, 2014, 5:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java, line 324
> > <https://reviews.apache.org/r/26232/diff/1/?file=710137#file710137line324>
> >
> > Does this work? I think something needs to 'pull' on the binding (by injecting) for anything to happen.
Not really. Fixed.
> On Oct. 1, 2014, 5:42 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java, line 161
> > <https://reviews.apache.org/r/26232/diff/1/?file=710137#file710137line161>
> >
> > We also need time-based retention. How about 1 month by default?
>
> Maxim Khutornenko wrote:
> Thought about that but was not sure the extra complexity is worth it. Also, don't we want to show update history in the UI regardless of how old it is?
>
> Bill Farner wrote:
> Given that it currently needs to fit in memory, i think not.
>
> For example: if i create a job to test something out, then kill it and never return. Probably shouldn't retain those updates forever.
>
> David McLaughlin wrote:
> Wouldn't the updates in that case be killed by the job garbage collection/cascading deletion of the job key?
>
> Bill Farner wrote:
> If so, that warrants validation in an integration test between TaskHistoryPruner, storage, and DbJobUpdateStore.
>
> Maxim Khutornenko wrote:
> We don't cascade deletes into the job_keys table. In fact, we don't actually delete from job_keys at all at this point. We should probably address this when converting TaskStore into H2.
>
> | if i create a job to test something out, then kill it and never return. Probably shouldn't retain those updates forever.
> Good point. This alone warrants adding time-based cleanup.
Added support for time-based pruning.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55088
-----------------------------------------------------------
On Oct. 1, 2014, 5:26 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2014, 5:26 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55088
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java
<https://reviews.apache.org/r/26232/#comment95448>
We also need time-based retention. How about 1 month by default?
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java
<https://reviews.apache.org/r/26232/#comment95449>
Does this work? I think something needs to 'pull' on the binding (by injecting) for anything to happen.
src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java
<https://reviews.apache.org/r/26232/#comment95450>
maxUpdatesPerJob?
src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java
<https://reviews.apache.org/r/26232/#comment95451>
Nit: starting this with "Pruning completed" breaks my brain a little. How about "Pruning job update history."
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java
<https://reviews.apache.org/r/26232/#comment95454>
I'm not sure how i feel about this. On one hand, it's nice that SQL handles a lot of the complexity. On the other, it would be _really_ nice to see deleted job update UUIDs in the log. Perhaps this function can return some details about the deleted updates?
- Bill Farner
On Oct. 1, 2014, 5:26 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 1, 2014, 5:26 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Oct. 2, 2014, 10:39 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java, line 89
> > <https://reviews.apache.org/r/26232/diff/4/?file=711547#file711547line89>
> >
> > if (!prunedUpdates.isEmpty()) {
> > LOG.info(...);
> > }
I actually want to log in either case for better transparency. Modified to support both cases.
> On Oct. 2, 2014, 10:39 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java, line 75
> > <https://reviews.apache.org/r/26232/diff/4/?file=711558#file711558line75>
> >
> > line break above
Done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55266
-----------------------------------------------------------
On Oct. 2, 2014, 7:54 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 7:54 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 7254588b55df9a12217c8ec172abc5976892497e
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55266
-----------------------------------------------------------
Ship it!
src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java
<https://reviews.apache.org/r/26232/#comment95651>
if (!prunedUpdates.isEmpty()) {
LOG.info(...);
}
src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java
<https://reviews.apache.org/r/26232/#comment95652>
line break above
- Bill Farner
On Oct. 2, 2014, 7:54 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 7:54 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 7254588b55df9a12217c8ec172abc5976892497e
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55322
-----------------------------------------------------------
Ship it!
Ship It!
- David McLaughlin
On Oct. 2, 2014, 10:51 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 10:51 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 2a81a94f99f242bbe400d8428ad1f1ce38a06a86
> src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java dbf0badbfcc19f40d9b9eeec22b7024d95a02884
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 6758b7b56e77882c67be2e39481ff76893ad1610
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 2, 2014, 11:16 p.m., David McLaughlin wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, lines 468-490
> > <https://reviews.apache.org/r/26232/diff/5/?file=713015#file713015line468>
> >
> > It'd be nice if this didn't just blanket delete all old updates, especially for active jobs. There are probably a certain class of service/cron that are rarely touched and it would be nice to keep around that change history. Would it add too much complexity to try and solve this?
>
> Maxim Khutornenko wrote:
> Well, that would require active task queries and implementation leaking outside DB layer. I'd rather not attempt anything like that until we move TaskStore into SQL.
Let's start with this policy and revisit. Maxim - mind dropping a TODO in DbJobUpdateStore to consider retaining at least the most recent update for all jobs?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55272
-----------------------------------------------------------
On Oct. 2, 2014, 10:51 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 10:51 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 2a81a94f99f242bbe400d8428ad1f1ce38a06a86
> src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java dbf0badbfcc19f40d9b9eeec22b7024d95a02884
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 6758b7b56e77882c67be2e39481ff76893ad1610
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Oct. 2, 2014, 11:16 p.m., David McLaughlin wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, lines 468-490
> > <https://reviews.apache.org/r/26232/diff/5/?file=713015#file713015line468>
> >
> > It'd be nice if this didn't just blanket delete all old updates, especially for active jobs. There are probably a certain class of service/cron that are rarely touched and it would be nice to keep around that change history. Would it add too much complexity to try and solve this?
Well, that would require active task queries and implementation leaking outside DB layer. I'd rather not attempt anything like that until we move TaskStore into SQL.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55272
-----------------------------------------------------------
On Oct. 2, 2014, 10:51 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 10:51 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 2a81a94f99f242bbe400d8428ad1f1ce38a06a86
> src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java dbf0badbfcc19f40d9b9eeec22b7024d95a02884
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 6758b7b56e77882c67be2e39481ff76893ad1610
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55272
-----------------------------------------------------------
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
<https://reviews.apache.org/r/26232/#comment95663>
It'd be nice if this didn't just blanket delete all old updates, especially for active jobs. There are probably a certain class of service/cron that are rarely touched and it would be nice to keep around that change history. Would it add too much complexity to try and solve this?
- David McLaughlin
On Oct. 2, 2014, 10:51 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 10:51 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 2a81a94f99f242bbe400d8428ad1f1ce38a06a86
> src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java dbf0badbfcc19f40d9b9eeec22b7024d95a02884
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 6758b7b56e77882c67be2e39481ff76893ad1610
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/
-----------------------------------------------------------
(Updated Oct. 2, 2014, 10:51 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
CR comments.
Bugs: AURORA-743
https://issues.apache.org/jira/browse/AURORA-743
Repository: aurora
Description
-------
The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 2a81a94f99f242bbe400d8428ad1f1ce38a06a86
src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java dbf0badbfcc19f40d9b9eeec22b7024d95a02884
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 6758b7b56e77882c67be2e39481ff76893ad1610
Diff: https://reviews.apache.org/r/26232/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/
-----------------------------------------------------------
(Updated Oct. 2, 2014, 7:54 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
CR comments.
Bugs: AURORA-743
https://issues.apache.org/jira/browse/AURORA-743
Repository: aurora
Description
-------
The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 7254588b55df9a12217c8ec172abc5976892497e
Diff: https://reviews.apache.org/r/26232/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/
-----------------------------------------------------------
(Updated Oct. 2, 2014, 7:40 p.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
CR comments.
Bugs: AURORA-743
https://issues.apache.org/jira/browse/AURORA-743
Repository: aurora
Description
-------
The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 7254588b55df9a12217c8ec172abc5976892497e
Diff: https://reviews.apache.org/r/26232/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Maxim Khutornenko
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java, line 85
> > <https://reviews.apache.org/r/26232/diff/2/?file=710417#file710417line85>
> >
> > I'm always nervous when important behavior is embedded in something seemingly-less-important like logging. Can you extract a variable to separate the two?
Kind of redundant but sure, done.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 140
> > <https://reviews.apache.org/r/26232/diff/2/?file=710418#file710418line140>
> >
> > "...last completed updates that completed less than {@code historyPruneThreshold} ago.."
Done.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 147
> > <https://reviews.apache.org/r/26232/diff/2/?file=710418#file710418line147>
> >
> > Keep the value rich as far down as you can, to mitigate accidental misuse:
> > Amount<Long, Time> historyPruneThreshold
Disagree. I don't really like Amount -> long ->Amount -> long dance that would require. The Amount is converted into long only once now, converted into an absolute timestamp and passed down to SQL to compare against.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java, line 141
> > <https://reviews.apache.org/r/26232/diff/2/?file=710419#file710419line141>
> >
> > s/result/pruned/
Sure.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java, line 143
> > <https://reviews.apache.org/r/26232/diff/2/?file=710419#file710419line143>
> >
> > How would you feel about making the two pruning goals distinct at the mapper level? Does that simplfiy anything?
> >
> > - get UUIDs of all updates older than pruneThreshold
> > - get UUIDs of the the last >retainCount updates for each job
> > - delete above UUIDs.
> >
> > I think i would find that easier to follow, at least.
This approach would require an extra SQL call (step 1) and potentially a lot more SQL calls in step 2 as we would now deal with raw job keys not pre-filtered for processing. The current algorithm is optimized to be less SQL-chatty and short circuits if there are no job keys to process:
- get job keys that have updates to be deleted (older than pruneThreshold and/or count > retainCount)
- for every key above get update UUIDs and delete them.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java, line 109
> > <https://reviews.apache.org/r/26232/diff/2/?file=710420#file710420line109>
> >
> > Avoid repeating the method signature in text:
> > s/Set of u/U/
Done.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java, line 121
> > <https://reviews.apache.org/r/26232/diff/2/?file=710420#file710420line121>
> >
> > How about `getPruneCandidates`?
I don't think it will be clear enough as we are selecting job keys rather then UUIDs here. How about selectJobKeysForPruning?
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java, line 320
> > <https://reviews.apache.org/r/26232/diff/2/?file=710421#file710421line320>
> >
> > Why this rather than an explicit delete record for the affected update UUIDs?
I thought about that but decided to stick with the current solution:
- less code to maintain: deleting a set of UUIDs would require a new JobUpdateStore API only used by the LogStorage
- less data to store: persisting a bunch of pruned UUIDs seems redundant where a a single prune call restores the consistency much more effectively.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java, line 318
> > <https://reviews.apache.org/r/26232/diff/2/?file=710421#file710421line318>
> >
> > If this comment remains, please elaborate on why it is not strictly necessary. (i know, but a future developer might not.)
Done.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java, line 45
> > <https://reviews.apache.org/r/26232/diff/2/?file=710425#file710425line45>
> >
> > Can you take a stab at using FakeScheduledExecutor instead of a real thread? I don't insist on it, but i'd like to see if that pattern works out in other situations.
> >
> > You'll have to modify FakeScheduledExecutor a bit to add support for `scheduleAtFixedRate`, but at that point all you should have to do in the test after `replay()` is
> >
> > clock.advance(pruneInterval);
That's a fine suggestion. Moved and refactored FakeScheduledExcecutor to support fixed rate scheduling.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55186
-----------------------------------------------------------
On Oct. 2, 2014, 1:23 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 1:23 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 147
> > <https://reviews.apache.org/r/26232/diff/2/?file=710418#file710418line147>
> >
> > Keep the value rich as far down as you can, to mitigate accidental misuse:
> > Amount<Long, Time> historyPruneThreshold
>
> Maxim Khutornenko wrote:
> Disagree. I don't really like Amount -> long ->Amount -> long dance that would require. The Amount is converted into long only once now, converted into an absolute timestamp and passed down to SQL to compare against.
>
> Bill Farner wrote:
> I'm actually suggesting you avoid the dance completely. Keep Amount<Long, Time> in the settings object, and only translate it to a long when you need a long. The only argument i can see against this is performance, which doesn't hold here.
Ah, in that case the comment was misplaced as I was reading that you wanted to make JobUpdateStore API Amount-aware :)
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55186
-----------------------------------------------------------
On Oct. 2, 2014, 7:40 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 7:40 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 7254588b55df9a12217c8ec172abc5976892497e
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Bill Farner <wf...@apache.org>.
> On Oct. 2, 2014, 2:54 a.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java, line 147
> > <https://reviews.apache.org/r/26232/diff/2/?file=710418#file710418line147>
> >
> > Keep the value rich as far down as you can, to mitigate accidental misuse:
> > Amount<Long, Time> historyPruneThreshold
>
> Maxim Khutornenko wrote:
> Disagree. I don't really like Amount -> long ->Amount -> long dance that would require. The Amount is converted into long only once now, converted into an absolute timestamp and passed down to SQL to compare against.
I'm actually suggesting you avoid the dance completely. Keep Amount<Long, Time> in the settings object, and only translate it to a long when you need a long. The only argument i can see against this is performance, which doesn't hold here.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55186
-----------------------------------------------------------
On Oct. 2, 2014, 7:40 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 7:40 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
> src/test/java/org/apache/aurora/scheduler/updater/FakeScheduledExecutor.java e35fe23f023f5acc6666fb2270a3634c91bec657
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 7254588b55df9a12217c8ec172abc5976892497e
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/#review55186
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java
<https://reviews.apache.org/r/26232/#comment95572>
I'm always nervous when important behavior is embedded in something seemingly-less-important like logging. Can you extract a variable to separate the two?
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java
<https://reviews.apache.org/r/26232/#comment95573>
"...last completed updates that completed less than {@code historyPruneThreshold} ago.."
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java
<https://reviews.apache.org/r/26232/#comment95574>
Keep the value rich as far down as you can, to mitigate accidental misuse:
Amount<Long, Time> historyPruneThreshold
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java
<https://reviews.apache.org/r/26232/#comment95575>
s/result/pruned/
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java
<https://reviews.apache.org/r/26232/#comment95580>
How would you feel about making the two pruning goals distinct at the mapper level? Does that simplfiy anything?
- get UUIDs of all updates older than pruneThreshold
- get UUIDs of the the last >retainCount updates for each job
- delete above UUIDs.
I think i would find that easier to follow, at least.
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
<https://reviews.apache.org/r/26232/#comment95576>
Avoid repeating the method signature in text:
s/Set of u/U/
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
<https://reviews.apache.org/r/26232/#comment95578>
s/Set of j/J/
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
<https://reviews.apache.org/r/26232/#comment95579>
How about `getPruneCandidates`?
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java
<https://reviews.apache.org/r/26232/#comment95582>
If this comment remains, please elaborate on why it is not strictly necessary. (i know, but a future developer might not.)
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java
<https://reviews.apache.org/r/26232/#comment95581>
Why this rather than an explicit delete record for the affected update UUIDs?
src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java
<https://reviews.apache.org/r/26232/#comment95583>
Can you take a stab at using FakeScheduledExecutor instead of a real thread? I don't insist on it, but i'd like to see if that pattern works out in other situations.
You'll have to modify FakeScheduledExecutor a bit to add support for `scheduleAtFixedRate`, but at that point all you should have to do in the test after `replay()` is
clock.advance(pruneInterval);
- Bill Farner
On Oct. 2, 2014, 1:23 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26232/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2014, 1:23 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-743
> https://issues.apache.org/jira/browse/AURORA-743
>
>
> Repository: aurora
>
>
> Description
> -------
>
> The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
> src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
> src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
> src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
> src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
> src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
> src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
>
> Diff: https://reviews.apache.org/r/26232/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 26232: Implementing update history pruner.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26232/
-----------------------------------------------------------
(Updated Oct. 2, 2014, 1:23 a.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
CR comments and log store support.
Bugs: AURORA-743
https://issues.apache.org/jira/browse/AURORA-743
Repository: aurora
Description
-------
The pruner runs on periodic basis and trims completed updates up to the guranteed per job retention count.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/async/AsyncModule.java aa45d27391b1786ca3d5e8c928045f1b6f3cf5ef
src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java ebae58a04e8857c5f26d4b57c27dfcda9e14c82c
src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java PRE-CREATION
src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java c3abffe575e801cebec3572cf4aceac83a238b55
src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 3db0114c5432a5b7d0b01d97c75494be9e3e99a3
src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java c583e085e0458835d51ebf740a3b5f01b428bb25
src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 66c91644677e7176ccf53dcfcf29a6792ec398bc
src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml 631ab2543dadb77b7fd1cae96adc9c0bd2f94b39
src/main/thrift/org/apache/aurora/gen/storage.thrift 7e502450f06abb449d06af09cc59185c6a9a2963
src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 011d9ec5d84c658c2b2e39bb0b3f9d20b5440cee
src/test/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPrunerTest.java PRE-CREATION
src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java 1b1160f349d64bbcd4d20103a82f3b8eb1ca75d9
src/test/java/org/apache/aurora/scheduler/storage/log/LogStorageTest.java 68df0d542e41438c0844f76fc5b9ec6996a00e8d
Diff: https://reviews.apache.org/r/26232/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Maxim Khutornenko