updating data sync models. adding results model #16
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
njmei
reviewed
Nov 9, 2024
Comment on lines
156
to
165
| @dataclass | ||
| class DataSyncResult(SchemaModel): | ||
| bytes_transferred: Optional[int] = None | ||
| files_transferred: Optional[int] = None | ||
|
|
||
| def add_bytes_transferred(self, bytes_transferred: int) -> None: | ||
| self.bytes_transferred = (self.bytes_transferred or 0) + bytes_transferred | ||
|
|
||
| def add_files_transferred(self, files_transferred: int) -> None: | ||
| self.files_transferred = (self.files_transferred or 0) + files_transferred |
There was a problem hiding this comment.
Is there a meaningful difference between having the default be None vs having bytes_transferred and files_transferred default to 0?
There was a problem hiding this comment.
I think the main difference is that it helps distinguish a result that explicitly tracks the bytes and files transferred vs a result where those things are not tracked. otherwise we might confuse ourselves with a result that says zero bytes are transferred and think something is wrong
There was a problem hiding this comment.
I think I would prefer for a data sync where "results are not tracked" to not return a DataSyncResult at all (and instead return None or something).
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.
What's in this Change?
Primarily meant to address the following bug - https://alleninstitute.atlassian.net/browse/OCSDV-354
DataSyncResultandBatchDataSyncResultPrimarily meant to address the following bug - https://alleninstitute.atlassian.net/browse/OCSDV-354
This also introduces a new approach to gathering results during data sync. Previously the data sync operations returned nothing and the lambda handlers would simply return the requests in the response. For a few reasons, this is not great and so I have introduced a more informative and cohesive result object that summarizes data sync operations based on bytes transferred, files transferred and for the batch case, the number of succeeded and failed tasks.
Connected PRs
Testing