Skip to content
This repository has been archived by the owner on Oct 14, 2023. It is now read-only.

Start of a basic implementation #1

Merged
merged 27 commits into from
Dec 9, 2022
Merged

Start of a basic implementation #1

merged 27 commits into from
Dec 9, 2022

Conversation

gaebel
Copy link
Contributor

@gaebel gaebel commented Sep 27, 2022

No description provided.

@gaebel gaebel changed the title Draft: Start of a basic implementation Start of a basic implementation Sep 27, 2022
@gaebel gaebel changed the title Start of a basic implementation Draft: Start of a basic implementation Sep 27, 2022
@gaebel
Copy link
Contributor Author

gaebel commented Nov 25, 2022

Current issue: Some DidRetry events get swallowed if I subscribe to onEvent in the tests.

Also, currently I keep the jobs alive on retry (which would mean that this job blocks the queue until it is successful). Would probably be better to terminate the task and enqueue a copy with a certain start time

Todo:

  • Proper access control
  • Proper Coroutine job queue
  • Remove delegates in favour of StateFlow

Todo (postponed):

  • Persistence (De-/Serialisation)
  • Queue / job recreation at app start
  • Concurrency
  • Timeout rule
  • Internet rule

@gaebel gaebel changed the title Draft: Start of a basic implementation Start of a basic implementation Nov 29, 2022
@gaebel
Copy link
Contributor Author

gaebel commented Dec 1, 2022

Allow defining exception types for repeat. Either via method or annotation (@Repeat(TestError::class))

@gaebel
Copy link
Contributor Author

gaebel commented Dec 1, 2022

Job cancellation lock blocks other cancellations

@gaebel
Copy link
Contributor Author

gaebel commented Dec 1, 2022

Cancelling the scope isn't a good idea. Look intoSupervisorJob and cancelChildren()

Copy link
Member

@benjohnde benjohnde left a comment

Choose a reason for hiding this comment

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

I like the general structure of it! In general it is JobScheduler holds a TaskFactory which knows how to deal with a given Task. From what I am seeing right now you probably could get rid of the TaskFactory and the Task itself could know how to instantiate itself with a given set of params. I mentioned at the top (cf. remarks in README.md) that you could describe the params via either interface or data class and by that give a clear definition how the params would look like. Then it could be a more generic create method which would not need a separate TaskFactory. Although this gives more freedom in the long run, I favour simplicity. But as I am only reviewing and have not your full thoughts - and probably missing the point -, what was the main reason to introduce the TaskFactory?

README.md Show resolved Hide resolved
@benjohnde
Copy link
Member

Regarding general persistence, could also be neat to have some super simple FileStorage (.json) instead of using the Preferences.

@benjohnde
Copy link
Member

Hm probably the simple file storage is not necessary, NSUserDefaults (on iOS) and SharedPreferences (on Android) are basically providing a persistent store for simple data (which is in the end file based).

@gaebel
Copy link
Contributor Author

gaebel commented Dec 5, 2022

Regarding general persistence, could also be neat to have some super simple FileStorage (.json) instead of using the Preferences.

Hm, yeah maybe the file system would be better. I guess UserDefaults would only be a problem if the JSON becomes too big. Nevertheless, I'll persist a map in UserDefaults to know which jobs have been persisted with which job persister (the job persister will be defined with an interface, that way you can just drop in your own implementation.).

@gaebel
Copy link
Contributor Author

gaebel commented Dec 5, 2022

Well, I guess I get rid of UserDefaults and persist all to disk. Do you think it's valuable to allow defining your own persister (maybe for encryption)?

@benjohnde
Copy link
Member

Well, I guess I get rid of UserDefaults and persist all to disk. Do you think it's valuable to allow defining your own persister (maybe for encryption)?

I don't know if there is any specific magic UserDefaults or SharedPreference do! IIRC UserDefaults actually just write a simple json to the Application Sandbox Folder.

@benjohnde
Copy link
Member

Should we proceed by merging this first implementation approach? I think we can build on this one and add future improvements in small increments, what do you think @gaebel?

@gaebel
Copy link
Contributor Author

gaebel commented Dec 9, 2022

Sure, it's in a working state 👍

@benjohnde
Copy link
Member

Perfect, will be merged :)

@benjohnde benjohnde merged commit b520ec7 into main Dec 9, 2022
@benjohnde benjohnde deleted the feat/simple-impl branch December 10, 2022 10:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants