-
-
Notifications
You must be signed in to change notification settings - Fork 2
Feature/nemo 205/architecture update #3
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
base: main
Are you sure you want to change the base?
Feature/nemo 205/architecture update #3
Conversation
89da293
to
f6e46ab
Compare
418d84c
to
4419ee5
Compare
4419ee5
to
d7dea90
Compare
|
||
// Then | ||
assertNotNull(sdk) | ||
assertEquals(sdk, TidepoolSDK.getInstance()) |
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 test pass? Not seeing getInstance()
in TidepoolSDK
?
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.
LGTM!
Noting a few known Limitations (To Be Addressed)
- Token refresh logic not implemented
- Several data schemas incomplete (marked with TODO)
- Limited error handling - generic exceptions only
- No retry logic for network failures
- Mapping boilerplate could use code generation
private var currentKey = initialKey | ||
|
||
/** Flag to prevent concurrent requests */ | ||
private var isMakingRequest = false |
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 is not thread-safe with coroutines
|
||
override suspend fun loadNextItems() { | ||
println("Paginator: loadNextItems() called with key: $currentKey, $isMakingRequest $isEndReached") | ||
if (isMakingRequest || isEndReached) { |
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.
Potential race condition here: two coroutines can both pass this check. Maybe we use a Mutex solution here? Something like:
private val requestMutex = Mutex()
override suspend fun loadNextItems() {
if (requestMutex.isLocked || isEndReached) return
requestMutex.withLock {
if (isEndReached) return@withLock
val result = onRequest(currentKey)
// ... handle result
}
}
val users: UserService by lazy { koin.get() } | ||
|
||
public fun shutdown() { | ||
stopKoin() |
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.
Won't this stop Koin for the entire application?
Can we use Koin's scope management instead of the global instance here?
class TidepoolSDK(
environment: Environment,
private val tokenProvider: TokenProvider,
) {
private val koinApp = koinApplication {
modules(
module {
single<EnvironmentInternal> { environment.toInternal() }
single<TokenProvider> { tokenProvider }
},
domainModule,
dataModule,
)
}
private val koin: Koin = koinApp.koin
val confirmations: ConfirmationService by lazy { koin.get() }
val data: DataService by lazy { koin.get() }
val metadata: MetadataService by lazy { koin.get() }
val users: UserService by lazy { koin.get() }
fun shutdown() {
koinApp.close() // Only closes this instance
}
}
https://tidepool.atlassian.net/browse/NEMO-205