Closed Paul-Hess closed 5 years ago
This looks ok to me other than Chris's comment. I'm a little worried that even if the test runs once a minute, we'll need to run it multiple times to ensure that the channel we're testing is picked up in a run (especially if the dev environments have a lot of channels). I need to think about that. My first proposal would have been to create an endpoint to trigger the s3 lifecycle reprocessing for a particular channel but I think the logic in this thing too muddy to allow that to be an easy change.
@lkemmerer I am going to make changes to some naming. The maxItems enforcer does not use the same "apportioning" strategy that the lifecycle rules TTL enforcer does, if that factored into the idea of an endpoint per channel. ~That said there is still a one minute timeout on each run which could still force some channels/items to be missed (I think the test might expose that behavior if true).~ The timeout is for aquiring the lock not on doing the work, so bonus. I have a Jira ticket to go into this deeper and if you think it is ok, I will defer further refactor to that larger task.
None the less, the naming of the property does not reflect that this file does two different distinct types of TTL enforcement and should change.
Oh! Cool, I was thinking the TTL enforcer and the max items enforcers both had that awful randomized channel selection. I think what you've done and what you are planning are right on. ✅
allows for scheduling maxItems cleanup to run more frequently so that it can be tested in a reasonable window of time. This is a spike for the sake of the tests, more refactoring in the "S3Config" and related classes is slated in more involved stories