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