Skip to content
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

feat: run background queue tasks #78

Merged
merged 21 commits into from
Dec 1, 2021
Merged

Conversation

levibostian
Copy link
Contributor

2nd milestone of the background queue: running tasks.

@levibostian levibostian marked this pull request as ready for review November 12, 2021 20:38
@RussellCIO RussellCIO added the iOS label Nov 15, 2021
Sources/Tracking/Background Queue/Queue.swift Show resolved Hide resolved
Sources/Tracking/CustomerIOImplementation.swift Outdated Show resolved Hide resolved
Sources/Tracking/Store/SdkConfig.swift Show resolved Hide resolved
Sources/Tracking/Background Queue/QueueRunner.swift Outdated Show resolved Hide resolved
Sources/Tracking/CustomerIO.swift Outdated Show resolved Hide resolved
@levibostian levibostian self-assigned this Nov 16, 2021
Copy link
Contributor

@hownowstephen hownowstephen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a couple things to note:

  1. Retrying of tasks needs to be nuanced. 50x errors should retry, 40x shouldn't since they have in some way been found unprocessable. Once we get to global error handling this can probably be handed off to the customer for input as well
  2. How much do we trust device timestamps? we're setting it to the timestamp at the time the request is queued. This could be a problem if the clock time is off by enough margin to have events treated as non-triggering due to device issues. A bit of an extreme case, but might be worth having a convo about whether we're comfortable using device time always / having a look at how other platforms do the same

docs/dev-notes/LINUX.md Outdated Show resolved Hide resolved
docs/dev-notes/LINUX.md Outdated Show resolved Hide resolved
init(
siteId: SiteId,
storage: QueueStorage,
runRequest: QueueRunRequest,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to @ami-aman and possibly @Shahroz16 on the readability side, as I'm still quite new to Swift and am definitely not the target audience here!

self.logger.verbose("background queue task \(nextTaskStorageId) success")

_ = self.storage.delete(storageId: nextTaskToRunInventoryItem.taskPersistedId)
case .failure(let error):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should be differentiating between issues in CIO (50x) and issues from the customer (40x)

runResults: newRunResults)
}

return goToNextTask()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a bit of an interesting questions as to what we should do if a request fails, I think it's probably fine to just continue in the queue due to how we handle customer creation, but worth having a think about the use case where there are two items in the queue:

request1 -> initial identify
request2 -> add device
request3 -> send event

if request1 fails and defers to later then the profile could in theory be empty and just contain a device until the queue is run again. Likely a small window, but it does raise the question as to whether the initial identify should be given a blocking behaviour 🤔 so that until it gets a non 50x response we don't continue. Or possibly any 50x stops the queue?

@levibostian levibostian merged commit 34c0f04 into background-q Dec 1, 2021
@levibostian levibostian deleted the background-q-runner branch December 1, 2021 20:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants