[BACK-2702] Allow closing datasets created by jellyfish so we can recalculate summaries correctly #677
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
toddkazakov
changed the title
darinkrauss
previously approved these changes
Oct 30, 2023
| return | ||
| // Allow closing datasets without deduplicator (i.e. those created by jellyfish) | ||
| // so we can mark summaries as outdated. | ||
| if dataSet != nil && dataSet.HasDeduplicatorName() { |
There was a problem hiding this comment.
If I'm understanding things correctly, I don't think this will cause issues with the current Jellyfish migration/deprecation plan. The plan does not currently include any updates to the data set (just data). Thus, a Jellyfish data set will not have a deduplicator name and so this code will not be executed.
If, however, we do need to update the data set with the deduplicator name, then this will cause a couple of potential issues:
- The Jellyfish data set and data may include some fields normally only present in Platform data (written by
DataRepository.UpdateDataSetandDataRepository.ActiveDataSetData). We'd need to do a survey of exactly what fields are added by Jellyfish or Platform and compare the two. Even if there are field differences they may not cause any real issue, just inconsistencies. - The Platform code that performs data duplication will execute on Jellyfish data (where other Jellyfish data is duplicated based upon this Jellyfish data using the Platform deduplication mechanism). I don't think this will cause an issue, though, since Jellyfish will have already deduplicated the data (by dropping any new duplicates) prior to closing of the Jellyfish data set. Thus, I think that this data deduplication step would end up just being a NOP.
In either case, we should verify this definitely won't cause issues and add it as one or more test cases, but at first glance there doesn't appear to be any (or if there are some they would be minor).
cc: @jh-bate
|
/query qa5 |
|
/query qa2 |
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.
Currently, jellyfish doesn't close legacy datasets after upload completion. Due to this, we can't detect when the upload has finished.
When a new batch is uploaded, jellyfish marks the summary as oudated with a 2 minute buffer, meaning the summary will be recalculated after 2 minutes. If an upload doesn't finish within this timeframe, the summary may be calculated mid-upload and may not be accurate.
In order to fix the issue above and to reduce the time required to recalculate a summary after an upload has finished and the subsequent push of updated reports to the EHR, this PR allows Uploader to "close" jellyfish datasets (which currently don't have a deduplicator) so summaries can be recalculated right after upload. This is important for in-clinic uploads.
This may have some unexpected side-effects when tidepool-org/jellyfish#195 is merged (e.g. running the deduplicator on jellyfish datasets).