Bulk Load CDK: Simply Interface & Add Check #45369
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
| @@ -19,7 +19,9 @@ import kotlinx.coroutines.sync.withLock | |||
| * TODO: Some degree of logging/monitoring around how accurate we're actually being? | |||
| */ | |||
| @Singleton | |||
| class MemoryManager(availableMemoryProvider: AvailableMemoryProvider) { | |||
| class MemoryManager( | |||
| private val availableMemoryProvider: AvailableMemoryProvider | |||
There was a problem hiding this comment.
This caused a micronaut bug -- the value wasn't getting injected without val
There was a problem hiding this comment.
Yeah live and learn
| @Singleton | ||
| @Requires(property = Operation.PROPERTY, value = "check") | ||
| @Requires(env = ["destination"]) | ||
| class CheckOperation( |
There was a problem hiding this comment.
any reason why we wouldn't want an abstract class here, with the check() as an abstract function
There was a problem hiding this comment.
The @Requires aren't inherited. It would force each implementor to specify the conditions under which their implementation is run.
| @@ -19,7 +19,9 @@ import kotlinx.coroutines.sync.withLock | |||
| * TODO: Some degree of logging/monitoring around how accurate we're actually being? | |||
| */ | |||
| @Singleton | |||
| class MemoryManager(availableMemoryProvider: AvailableMemoryProvider) { | |||
| class MemoryManager( | |||
| private val availableMemoryProvider: AvailableMemoryProvider | |||
| */ | ||
| interface Destination { | ||
| interface DestinationWrite { |
There was a problem hiding this comment.
not in love with the name. DestinationWriteOperation?
There was a problem hiding this comment.
NVM, There s a WriteOperation down below. Begs the question why we need both though
There was a problem hiding this comment.
See previous comment.
| suspend fun teardown(succeeded: Boolean = true) {} | ||
| } | ||
|
|
||
| @Singleton | ||
| @Secondary | ||
| class DefaultDestination(private val streamLoaderFactory: StreamLoaderFactory) : Destination { | ||
| class DefaultDestinationWrite : DestinationWrite { |
There was a problem hiding this comment.
do we need a default that crashes? Might be worth explaining why
There was a problem hiding this comment.
Can't hurt to tell the implementor what's missing 🤷
johnny-schmidt
force-pushed
the
issue-9746/simplify-interface-add-check
branch
from
fa5f5d8 to
8630887
Compare
What