-
Notifications
You must be signed in to change notification settings - Fork 2
feat(act_executor): Added strategies for incoming event #4
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: master
Are you sure you want to change the base?
feat(act_executor): Added strategies for incoming event #4
Conversation
fix(LiveDataAct): fix duplicate act when generic type isn't supported
| * Save and execute only one event and ignore all new events. | ||
| * After event is executed, it becomes possible to add new event. | ||
| * | ||
| * REPLACE - Only one event can be stored at the same time. Replace already added event and ignore all new events. |
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.
ты тут не игнорируешь ничего
| private fun startAct(act: Act) { | ||
| when { | ||
| actMap.contains(act.id) -> Timber.d("id: ${act.id} - Act duplicate") | ||
| actMap.size() == 1 -> Timber.d("Only one event can be processed at the same time") |
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.
это в орне не верно, у тебя может быть сколько угодно событий в map
| private fun startReplaceAct(act: Act) { | ||
| when { | ||
| actMap.contains(act.id) -> actMap.replace(act.id, act) | ||
| actMap.size() == 1 -> Timber.d("Only one event can be processed at the same time") |
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.
это в орне не верно, у тебя может быть сколько угодно событий в map
| } | ||
| private fun startAct(act: Act) { | ||
| when { | ||
| actMap.contains(act.id) -> Timber.d("id: ${act.id} - Act duplicate") |
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.
много дублирования
| } | ||
| private fun startAct(act: Act) { | ||
| when { | ||
| actMap.contains(act.id) -> Timber.d("id: ${act.id} - Act duplicate") |
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.
- мне кажется что тут будет неожиданное поведение
| actDispatcher: ActDispatchers = ActDispatchers.DEFAULT | ||
| ) : ActExecutorInterface { | ||
|
|
||
| private val scope: CoroutineScope = CoroutineScope(actDispatcher.dispatcher) |
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.
зачем нам глобальная корутина для инструмента?
| val invokeTime = measureTimeMillis { act.actFunction() } | ||
| job = scope.launch { | ||
| delay(act.delay - invokeTime) | ||
| removeFromMap() |
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.
job?.cancel() не нужен, у тебя корутина закончит свое выполнение как только будет вызван removeFromMap()
| delay(act.delay - invokeTime) | ||
| removeFromMap() | ||
| throw IllegalArgumentException("Type T in LiveData<T> unregistered") | ||
| job?.cancel() |
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.
а вот для replace - уже будет нужен
|
|
||
| private fun startReplaceAct(act: Act) { | ||
| when { | ||
| actMap.contains(act.id) -> actMap.replace(act.id, act) |
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.
ты подменяешь act и на этом все. никакой логики по replace нет
| */ | ||
|
|
||
| enum class ActStrategy() { | ||
| DEFAULT, REPLACE, QUEUE |
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.
давай пока откажемся от QUEUE - так будет проще. потом следующей задачей его внесем
No description provided.