-
Notifications
You must be signed in to change notification settings - Fork 1
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
Adding chron job for snap application transmission #496
Conversation
sree-cfa
commented
Jan 5, 2024
•
edited
Loading
edited
- Added hourly cronjob for SNAP app transmission
- Clean up, logging for SNAP transmission
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.
Looks good to me. Just added a few questions/comments for my curiousity.
public void transferSubmissions() { | ||
// Get submissions to transfer | ||
String batchSeq = Long.toString(transmissionRepository.nextValueBatchSequence()); | ||
String batchIndex = Strings.padStart(batchSeq, BATCH_INDEX_LEN, '0'); | ||
UUID uuid = UUID.randomUUID(); |
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.
Saw this and it made me curious about UUID collisions. Quick research suggest that it is extremely improbable and therefore unlikely.
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.
Yeah - unlikely and also ok if theres collisions, ideally I'd like to track "batchIndex" in the "run_id" column, but since it's restricted to UUID, Im just generating a new one here and logging the batchIndex+runId
|
||
transmission.setDocumentationErrors(Map.of("error", e.getMessage())); | ||
transmission.setStatus(TransmissionStatus.Failed); | ||
updateTransmission(uuid, transmission); |
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.
@sree-cfa - curious if it's worthwhile including the subfolder details to hone in on what might cause the error.
I realize that the whole transmission would fail but I am curious if we will ever end up with big enough transmissions that it might become difficult to troubleshoot.
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.
Oh, yeah good call out - will add that
String day = (String) inputData.get("birthDay"); | ||
String month = (String) inputData.get("birthMonth"); | ||
String year = (String) inputData.get("birthYear"); | ||
String day = (String) inputData.getOrDefault("birthDay", ""); |
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.
I am surprised that we need a default for birthday since it is a required field. Wondering if this allows partial applications to be submitted (if there is a split session issue for example). 🤔
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.
Yeah - unlikely it will be null, but just added it as a safeguard