-
Notifications
You must be signed in to change notification settings - Fork 7
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
LOOP-1169 - Upload device logs #100
Conversation
await tapi.setLogging(self) | ||
await tapi.addObserver(self) |
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.
Do these need to run sequentially or in this particular order? If not, we could run them simultaneously to get a tad bit more performance with a TaskGroup
.
await tapi.setLogging(self) | |
await tapi.addObserver(self) | |
await withTaskGroup { group in | |
group.addTask { await tapi.setLogging(self) } | |
group.addTask { await tapi.addObserver(self) } | |
await group.waitForAll() | |
} |
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'd be very surprised if this created any measurable performance improvement. Combining that with the additional code and complexity and loss of readability, I don't think it would be worth it, especially for a situation where performance isn't important in the first place.
let _ = try await createData(created.flatMap { $0.data(for: userId, hostIdentifier: hostIdentifier, hostVersion: hostVersion) }) | ||
let _ = try await deleteData(withSelectors: deleted.flatMap { $0.selectors }) |
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.
Similar to above. Another possible use of TaskGroup
?
let _ = try await createData(created.flatMap { $0.data(for: userId, hostIdentifier: hostIdentifier, hostVersion: hostVersion) }) | |
let _ = try await deleteData(withSelectors: deleted.flatMap { $0.selectors }) | |
await withThrowingTaskGroup(of: Void.self) { group in | |
group.addTask { try await createData(created.flatMap { $0.data(for: userId, hostIdentifier: hostIdentifier, hostVersion: hostVersion) }) } | |
group.addTask { try await deleteData(withSelectors: deleted.flatMap { $0.selectors }) } | |
await group.waitForAll() | |
} |
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.
Serialization is needed here; some of the deletes may be for objects created in the createData line.
let _ = try await createData(created) | ||
let _ = try await updateData(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.
Similar to above. Another possible use of TaskGroup
?
let _ = try await createData(created) | |
let _ = try await updateData(updated) | |
await withThrowingTaskGroup(of: Void.self) { group in | |
group.addTask { try await createData(created) } | |
group.addTask { try await updateData(updated) } | |
await group.waitForAll() | |
} |
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.
Serialization is intended here.
do { | ||
// TODO: fetching logs is not implemented on the backend yet: awaiting https://tidepool.atlassian.net/browse/BACK-3011 | ||
// For now, we expect this to error, so the catch has been modified to break out of the loop. Once this is implemented, | ||
// We will want to retry on error, so the break should eventually be removed. |
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 this mean that the upload will potentially upload redundant data each time Tidepool Loop is restarted?
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, I noted this in the ticket.
https://tidepool.atlassian.net/browse/LOOP-1169