[GH-1465] Specify TDR Polling Interval #542
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Thank you for addressing this! Two additional pieces of feedback:
pollingInterval/polling_intervalwithout the unit of measurement is ambiguous. My preference is to make explicit reference in naming so that there's no question of how the value is interpreted, ex.pollingIntervalMinutes/polling_interval_minutesbut this may be subjective.- Please also update the GitHub docs to reflect this new configuration.
api/src/wfl/source.clj
Outdated
|
|
||
| (defn ^:private tdr-source-should-poll? | ||
| "Return true if it's been at least `tdr-source-polling-interval-minutes` | ||
| "Return true if it's been at least the specified `polling_interval` or, if unspecified, `tdr-source-default-polling-interval-minutes` |
There was a problem hiding this comment.
Nit: easier to read if broken up with max 80 chars per line. Ex.
"Return true if it's been at least the specified `polling_interval`
or, if unspecified, `tdr-source-default-polling-interval-minutes`
since `last_checked` -- when we last checked for new rows in the TDR."
| runInTransaction="false"> | ||
| <comment> | ||
| We want to be able to allow each terra data repo source to be able to specify | ||
| the rate in which they poll for a new snapshot. The new column added to TerraDataRepoSource |
There was a problem hiding this comment.
Suggest rewording:
"the rate in which they poll for a new snapshot"
To:
"the rate at which they poll TDR for new rows to snapshot"
api/src/wfl/source.clj
Outdated
Comment on lines
317
to
319
| (let [checked (timestamp-to-offsetdatetime last_checked) | ||
| minutes-since-poll (.between ChronoUnit/MINUTES checked utc-now)] | ||
| minutes-since-poll (.between ChronoUnit/MINUTES checked utc-now) | ||
| polling-interval (or pollingInterval tdr-source-default-polling-interval-minutes)] |
There was a problem hiding this comment.
Style nit: it's a bit easier to read when we align the assignments.
(let [checked (timestamp-to-offsetdatetime last_checked)
minutes-since-poll (.between ChronoUnit/MINUTES checked utc-now)
polling-interval (or pollingInterval
tdr-source-default-polling-interval-minutes)]
| @@ -95,6 +97,11 @@ The email addresses of those whom should be "readers" of all snapshots created | |||
| by workflow-launcher in this workload. You can specify individual users and/or | |||
| Terra/Firecloud groups. | |||
|
|
|||
| #### `pollingIntervalMinutes` | |||
There was a problem hiding this comment.
Thanks for adding this!
You may also be able to remove this block higher up.
okotsopoulos
approved these changes
Dec 3, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
Currently the polling interval for a Terra DataRepo Source is 20 minutes. This could be too long for some cases and certainly makes the system tests run long. This PR allows the workload creator to specify a polling interval and defaults to 20 minutes if it is not provided.
https://broadinstitute.atlassian.net/browse/GH-1465
Changes
Review Instructions
Run the system test for
test-workload-sink-outputs-to-tdr