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.
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
feat: run background queue tasks #78
Changes from 7 commits
506d8ec
7730358
2f90a19
9a4bdac
bbccbb8
7f49766
197541e
4ab3b6f
f7bfa97
bce02bb
96956c7
88c626b
2980870
21a5ded
018b666
71276bd
b998fa4
7aa5197
2db66c6
779cf7e
211285f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
this runner + runRequest + Manager pattern feels like it could all be flattened into just the runner - is there some specific benefit you're aiming for by breaking them apart?
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 did try to add comments to each of the queue classes (queue, run request, runner, storage, manager) explaining the use case of each.
Breaking the queue logic into separate files are for
QueueRequestManager
class to keep singletons as small as possible.I want to avoid the queue or queue runner being the singleton since the runner can have many dependencies that it would have a strong reference to. It needs strong references since the runner performs async operations and weak references would be GCed before the
onComplete
callback is called in the runner.Now that I think about it, if we were to keep the runner as it is today where it's just a place where HTTP requests are performed, then we may not have to worry about the runner having many dependencies because it's just the
HttpClient
it's having a dependency on. My concern was that as the SDK grows, we may break out that runner HTTP logic of the runner into using a Repository or other design pattern. When we would do this, the dependencies list can grow greatly as the repository may have lots of children dependencies themselves.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.
found it a little hard to differentiate between them (re: readability) and understand their purpose. Of the three reasons for breaking, memory safety though is a trump card - assuming that doing it in a flat way would increase likelihood that we could become more of a mem 🐷 - let's get a second opinion on the strategy here from @ami-aman
re: keeping the runner as a pure http client, I think the question ends up being whether there are any things we've discussed adding to our SDK that would require different behaviour than that? I haven't seen anything, and would err on the side of handling our current known & planned needs, vs worrying too much about long term. If we need to do a major refactor in future to handle some fancier use case, we can definitely do that, not worth encoding complexity now for an unknown future
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.
Though there is nothing wrong with modulating the files into Manager, runner and handler. @levibostian has focussed on keeping the code segregated / modulated + clean considering the future of the SDK and also I believe, testability since testing gets easier and reliable with modulated code. I appreciate Levi's efforts in this.
But at this moment, as @hownowstephen said, when we are introducing Alphas and Betas and we are not even sure what exactly is the customer needs OR how would our releases be welcomed or accepted I think we should focus more on getting the work done and bringing out new features and understand what next would the customer want. As we introduce new features in our SDKs, code will be re-written sometimes or might (at least) need major/minor tweaks. So, we will have time to re-consider the code not modulated. And revisiting the code base later will also help us find out our own mistakes (or can call it as scope of improvement) which we, the developers usually miss out while development.
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 just took another glance at the files that make up the queue. Here they are:
QueueRequestManager
- the small singletonQueueRunner
- contains 1 function that gets called to run each of the queue tasks.QueueRunRequest
- The logic of the queue.QueueStorage
- wrapper around the file system to read/write data to files.Queue
- a delegate that is the entry point in the SDK to communicate with the SDK and all of it's features.Thinking about the above list, I don't see any value in combining any of the classes. Every scenario I have thought through in my head I am hesitant because of 1 of the 3 issues I pointed out previously.
I can see something like combining
QueueRunRequest
andQueueRunner
perhaps if we were able to think of a better replacement to the wayQueueRunner
.runTask()` works today (Maybe there is a way to reduce the copy/paste networking code that is in the runner today?).Are we on the same page or am I talking about something completely off?
I did add comments explaining the value of the
QueueRequestManager
being small.I am bias because I wrote the code, haha! but would another solution to this problem be to write some high level overview docs about the background queue and how it works? Would that help to make this code quicker to read in the future? I find it a red flag when we come up with the idea of docs explaining code over just making the code more readable in the first place, but I currently dont have major ideas for making the code more readable?
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'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!
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.
we should be differentiating between issues in CIO (50x) and issues from the customer (40x)
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.
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 identifyrequest2
-> add devicerequest3
-> send eventif
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?