-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EVA-3570 Add jobs for recovering blocks through recovery agent for category ss and rs #447
EVA-3570 Add jobs for recovering blocks through recovery agent for category ss and rs #447
Conversation
878025c
to
e7a4b58
Compare
clusteredVariantEntityList.add(entity); | ||
} | ||
|
||
// Entries for 2nd block |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Entries for 2nd block | |
// Entries for 2nd block -- Missing 6 RS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, really appreciate the detailed explanations in the tests.
A couple naming suggestions (of course) but otherwise looks good to me.
@@ -1,3 +1,4 @@ | |||
recovery.cutoff.days=14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it also work to leave the property empty when it's not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we don't need to provide this for other jobs.
|
||
import java.time.LocalDateTime; | ||
|
||
public class MonotonicAccessionRecoveryAgentCategoryRSService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because we promised we'd argue about names... what about something shorter like RSAccessionRecoveryService
? And analogous naming throughout the other classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
VALUES (4, 'test-instance-recover-state-00', 'rs', 3000000090, 3000000089, 3000000119, true, '1970-01-01 00:00:00'); | ||
|
||
INSERT INTO contiguous_id_blocks | ||
VALUES (5, 'test-instance-recover-state-00', 'rs', 3000000120, 3000000119, 3000000149, true, '2099-01-01 00:00:00'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't wait for this test to fail in 2100...
(Just kidding, if the code we write lasts that long I'll be impressed...)
|
||
@Test | ||
@DirtiesContext | ||
public void testContiguousBlocksAreReleasedInCaseOfJobFailures() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right test name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected
|
||
@Test | ||
@DirtiesContext | ||
public void testContiguousBlocksAreReleasedInCaseOfJobFailures() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above - think this test name should be different...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
corrected
fca29b0
to
7b2780a
Compare
No description provided.