Before this PR: This is a bit early, but the single bucket sweep task / foreground task doesn't have tests.
After this PR:
The single bucket sweep task / foreground task now has integration tests.
Priority: P2
Concerns / possible downsides (what feedback would you like?):
I don't like making some of the sweep primitives e.g. SweepQueueWriter public, but I don't see a good alternative - either we have to lose the asts package entirely, or we define the integration test not in asts and then make relevant classes from asts public, or we declare some super janky bridging classes in the test code, and of these options I found this to be the least unpalatable.
The tests are quite heavy and not the easiest to read, though given the criticality of the classes involved I think it's worth the complexity.
Is documentation needed?: No
Compatibility
Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?: No
Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?: No
The code in this PR may be part of a blue-green deploy. Can upgrades from previous versions safely coexist? (Consider restarts of blue or green nodes.): Yes
Does this PR rely on statements being true about other products at a deployment - if so, do we have correct product dependencies on these products (or other ways of verifying that these statements are true)?: No
Does this PR need a schema migration? No
Testing and Correctness
What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?: Nothing in particular
What was existing testing like? What have you done to improve it?: Most of this PR is focused on tests.
If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.: N/A
If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?: N/A
Execution
Test change not expected to have production impacts.
Scale
Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.: No
Would this PR be expected to perform a large number of database calls, and/or expensive database calls (e.g., row range scans, concurrent CAS)?: I don't think so
Would this PR ever, with time and scale, become the wrong thing to do - and if so, how would we know that we need to do something differently?: Probably not unless we upgrade to a version of java that has clever typing or something like that.
Development Process
Where should we start reviewing?: DefaultSingleBucketSweepTask, then the tests.
If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?: The integration tests as a unit are >700 of the diff; these could be split by individual test methods though I'm not sure that's valuable.
Please tag any other people who should be aware of this PR:
@jeremyk-91
@sverma30
@raiju
General
Before this PR: This is a bit early, but the single bucket sweep task / foreground task doesn't have tests.
After this PR: The single bucket sweep task / foreground task now has integration tests.
Priority: P2
Concerns / possible downsides (what feedback would you like?):
public, but I don't see a good alternative - either we have to lose theastspackage entirely, or we define the integration test not inastsand then make relevant classes fromastspublic, or we declare some super janky bridging classes in the test code, and of these options I found this to be the least unpalatable.Is documentation needed?: No
Compatibility
Does this PR create any API breaks (e.g. at the Java or HTTP layers) - if so, do we have compatibility?: No
Does this PR change the persisted format of any data - if so, do we have forward and backward compatibility?: No
The code in this PR may be part of a blue-green deploy. Can upgrades from previous versions safely coexist? (Consider restarts of blue or green nodes.): Yes
Does this PR rely on statements being true about other products at a deployment - if so, do we have correct product dependencies on these products (or other ways of verifying that these statements are true)?: No
Does this PR need a schema migration? No
Testing and Correctness
What, if any, assumptions are made about the current state of the world? If they change over time, how will we find out?: Nothing in particular
What was existing testing like? What have you done to improve it?: Most of this PR is focused on tests.
If this PR contains complex concurrent or asynchronous code, is it correct? The onus is on the PR writer to demonstrate this.: N/A
If this PR involves acquiring locks or other shared resources, how do we ensure that these are always released?: N/A
Execution
Test change not expected to have production impacts.
Scale
Would this PR be expected to pose a risk at scale? Think of the shopping product at our largest stack.: No
Would this PR be expected to perform a large number of database calls, and/or expensive database calls (e.g., row range scans, concurrent CAS)?: I don't think so
Would this PR ever, with time and scale, become the wrong thing to do - and if so, how would we know that we need to do something differently?: Probably not unless we upgrade to a version of java that has clever typing or something like that.
Development Process
Where should we start reviewing?: DefaultSingleBucketSweepTask, then the tests.
If this PR is in excess of 500 lines excluding versions lock-files, why does it not make sense to split it?: The integration tests as a unit are >700 of the diff; these could be split by individual test methods though I'm not sure that's valuable.
Please tag any other people who should be aware of this PR: @jeremyk-91 @sverma30 @raiju