Add onSync callback for implementing action queues by VladBrok · Pull Request #52 · logux/core

VladBrok · 2023-08-04T08:32:39Z

This PR adds "onSync" callback that can be used in @logux/server to implement action queues.

…to finish in queue before destroy

ai · 2023-08-22T21:17:51Z

-        })
+    let process = processAction.bind(this)
+    if (this.options.onActions) {
+      this.options.onActions(process, action, meta)


sendSynced will be sent just after onActions call. Don’t we need to do await or something?

Also, we need to be ready for errors in onActions.

onActions is not async so I didn't add an await

То есть ты хочешь переделать протокол и отправлять synced не после inMap, inFilter и onAction (как сейчас) а сразу?

Почему такое изменение хочешь? Какие сайд-эффекты?

Внутри inFilter происходит вызов коллбека access, и я подумал, что его лучше тоже вызывать в очереди, чтобы было так:

выполняется action_1.access

выполняется action_1.process

выполняется action_2.access

выполняется action_2.process

Про изменение протокола не подумал. Может, лучше сделать, что onActions будет вызываться вместо добавления action в лог? То есть сервер будет на onActions добавлять action в лог в порядке очереди, а inMap и inFilter будут вызываться как сейчас

ai · 2023-08-22T21:19:53Z

  equal(pair.rightNode.log.actions(), [{ type: 'a1' }])
 })

+test('calls onActions if present', async () => {


I don’t see how onActions works in a test. I am not sure that we are property testing it.

ai · 2023-08-22T21:21:35Z

  inMap?: LogMapper

+  /**
+   * Function that will be called before node sends 'synced'


It is unclear how to use it and why. We need to say a little more details here.

ai · 2023-08-22T21:23:14Z

Can you explain me this API? I didn’t get what it ignoreDestroying and how to use onActions.

I tried to look at Logux Server code, but it didn’t help any Logux Core changes should be self-explained.

You can switch to Russian if it will be simpler.

VladBrok · 2023-08-23T15:22:27Z

Can you explain me this API? I didn’t get what it ignoreDestroying and how to use onActions.

I tried to look at Logux Server code, but it didn’t help any Logux Core changes should be self-explained.

You can switch to Russian if it will be simpler.

Спасибо за ревью.

На данный момент, для каждого action, вызывается inMap, затем inFilter, затем action добавляется в лог. При добавлении action в лог, сервер начинает его обрабатывать.

API, которое я добавил, позволяет вызывать inMap, inFilter и добавление action в лог тогда, когда будет нужно. В нашем случае, это будет происходить в очереди.

То есть:

Вызывается onActions(process, action, meta); функция process содержит вызовы inMap, inFilter и добавление action в лог
В onActions, сервер добавляет process, action и meta в очередь.
Очередь вызывает process(action, meta), ждет пока action обработается (on('processed') или on('error')), и после этого переходит к следующему action.

ignoreDestroying нужен для того, чтобы завершить выполнение всех action в очереди перед выключением сервера. Используется здесь: https://github.com/logux/server/pull/146/files#diff-dcb9d56556bf3885bd0d0fd9d88b318f0bdeddd59f85fe50c319ae959bfebd85R230. Без него может получится так:

В очереди 2 action, первый обрабатывается
Сервер начал выключаться
Первый action обработался, очередь переходит ко второму
При добавлении второго action в лог, он не обработается, т.к. стоит проверка if (this.destroying) { return }

ai · 2023-08-23T15:31:49Z

Вызывается onActions(process, action, meta); функция process содержит вызовы inMap, inFilter и добавление action в лог

Всё равно не понимаю API. Дай примеры на псевдо-коде как её использовать (абстрактно, вне сервера).

ignoreDestroying нужен для того, чтобы завершить выполнение всех action в очереди перед выключением сервера.

А мы можем избежать вставки ignoreDestroying? Например, изменить meta через какой-то другой API?

VladBrok · 2023-08-23T17:21:18Z

Вызывается onActions(process, action, meta); функция process содержит вызовы inMap, inFilter и добавление action в лог

Всё равно не понимаю API. Дай примеры на псевдо-коде как её использовать (абстрактно, вне сервера).

ignoreDestroying нужен для того, чтобы завершить выполнение всех action в очереди перед выключением сервера.

А мы можем избежать вставки ignoreDestroying? Например, изменить meta через какой-то другой API?

Примеры использования:

onActions(process, action, meta) {
  myActionQueue.schedule(async () => {
    await process(action, meta)
  })
}

```ts
onActions(process, action, meta) {
  myActions.push({process, action, meta})
  if (myActions.length === 10) {
     myActions.forEach(async ({process, action, meta}) => {
       await process(action, meta)
    })
    myActions.clear()
  }
}

ai · 2023-08-23T17:31:48Z

И как эти примеры будут работать? Объясни когда вызовутся inMap, когда сработает колбэк и т. п.

ai · 2023-08-23T17:32:59Z

И объясни почему в API там process() а не, например, возвращение [action, meta].

VladBrok · 2023-08-23T18:10:22Z

Добавил комментарии:

onActions(process, action, meta) {
  // Вызываем process(action, meta) не сразу, а сохраняем в очередь
  // myActionQueue будет вызывать коллбеки по очереди, а не параллельно
  myActionQueue.schedule(async () => {
    await process(action, meta) // Вызовет inMap, inFilter и добавить action в лог
  })
}

onActions(process, action, meta) {
  // Вызываем process(action, meta) не сразу, а когда накопиться 10 action'ов
  myActions.push({process, action, meta})
  if (myActions.length === 10) {
     myActions.forEach(async ({process, action, meta}) => {
       await process(action, meta) // Вызовет inMap, inFilter и добавить action в лог 
    })
    myActions.clear()
  }
}

В API там process, потому что внутри него вызываются InMap, inFilter и происходит добавление action в лог. Получается что нужно вызвать process(action, meta) и он вызовет inMap и inFilter.

ai · 2023-08-23T18:16:42Z

А почему нельзя вернуть Промис и вызвать inMap и inFilter в Promise.then()?

ai · 2023-08-23T18:17:00Z

И в чём тогда разница с inMap?

VladBrok · 2023-08-25T17:40:06Z

Поменял, чтобы inMap и inFilter вызывались, как сейчас, а onActions будет вызываться вместо добавления action в log (сервер сам будет добавлять action в лог, но уже в очереди, а не сразу).
Поменял API, вот пример:

  onActions(action, meta) {
    // Add action to the log later
    myActionQueue.schedule(() => {
      actionLog.add(action, meta)
    })
  }

ai · 2023-08-26T22:28:16Z

+  /**
+   * Function that will be called instead of adding action to the log
+   * after inMap and inFilter have been called.
+   * Use it if you want more control over when an action will be added to the log


Suggested change

* Use it if you want more control over when an action will be added to the log

*

* Use it if you want more control over when an action will be added to the log.

ai · 2023-08-26T22:28:39Z

+   * Function that will be called instead of adding action to the log
+   * after inMap and inFilter have been called.
+   * Use it if you want more control over when an action will be added to the log
+   * @example


The project uses different syntax for examples in TypeDoc

ai · 2023-08-29T20:41:00Z

+   * }
+   * ```
+   */
+  onActions?: ActionsCallback


There are two problems with naming:

onActions has s on the end, but we call it for single action.

log.add() also add “action”, why we are not calling onAction there?

What do you think about onSync?

ai · 2023-08-29T20:42:41Z

+  equal(catched, [error])
+})
+
+test('actions are added to the log if onActions is present and if actions were added to the log inside it', async () => {


Too long line. Let’s name shorter to avoid lines > 80 symbols.

ai · 2023-08-29T20:43:10Z

+test('actions are added to the log if onActions is present and if actions were added to the log inside it', async () => {
+  let pair = await createTest(created => {
+    created.rightNode.options.inFilter = async (action, meta) => {
+      type(meta.id, 'string')


You can remove this checks since we are not testing inFiler and inMap

ai · 2023-08-29T20:43:54Z

+      return [{ type: action.type + '1' }, meta]
+    }
+    created.rightNode.options.onActions = (action, meta) => {
+      created.rightNode.log.add(action, meta)


This test will not find that onActions is not calling (because log.add is default behavior)

For instance, let’s put actions to an array and then check the array that it contains actions and log is empty.

ai · 2023-08-29T20:46:03Z

+  equal(pair.rightNode.log.actions(), [{ type: 'a1' }, { type: 'b1' }])
+})
+
+test('actions are not added to the log if onActions is present and if actions were not added to the log inside it', async () => {


We don’t need this test if we will check array filling in test above

Co-authored-by: Andrey Sitnik <andrey@sitnik.ru>

ai · 2023-08-29T21:06:29Z

+test('onSync is called instead of adding an action to the log', async () => {
+  let actions = []
+  let pair = await createTest(created => {
+    created.rightNode.options.onSync = (action, meta) => {


I mean let’s remove type(action, 'object') from inMap/inFilter.

The inMap/inFilter were useful to test that we call onSync after them.

VladBrok · 2023-08-31T09:09:26Z

I've changed an API in order to support calling access in a queue.
onSync now has additional parameter called processAction.
processAction is an async function that calls inMap, inFilter, and Log#add. Server will call it in a queue.
Example:

onSync(processAction, action, meta) {
 // Process an action later
 myActionQueue.schedule(async () => {
   await processAction(action, meta) // will call `inMap`, `inFilter` and `Log#add`
 })
}

ai · 2023-09-01T09:34:42Z

+   * }
+   * ```
+   */
+  onSync?: SyncCallback


I have plan later to replace inFiler/inMap with onSync (are you agree that it is possibe?)

Can you rename onSync to onReceive (so later we will be able to add onSend for outFilter/outMap)?

Yes this should be possible

* add onActions callback for implementing action queues * add ignoreDestroying to meta in order to support waiting for actions to finish in queue before destroy * add test for ignoreDestroying, export ActionsCallback type * handle error in onActions * improve onActions description * add 1 more test for onActions * remove ignoreDestroying, move related logic to logux server * use different code example for onActions * fix typo * call inMap and inFilter outside of onActions * review docs issue * Update base-node/index.d.ts Co-authored-by: Andrey Sitnik <andrey@sitnik.ru> * Update base-node/index.d.ts Co-authored-by: Andrey Sitnik <andrey@sitnik.ru> * rename onActions to onSync, improve onSync tests * add inMap and inFilter to the onSync test * change onSync api to support calling access inside of queue * rename onSync to onReceive --------- Co-authored-by: Andrey Sitnik <andrey@sitnik.ru>

VladBrok added 2 commits August 4, 2023 11:15

add onActions callback for implementing action queues

3a669db

Merge branch 'main' into feat/54

703540e

VladBrok mentioned this pull request Aug 6, 2023

Add action processing queues logux/server#146

Merged

VladBrok added 2 commits August 7, 2023 16:41

add ignoreDestroying to meta in order to support waiting for actions …

55d97bd

…to finish in queue before destroy

add test for ignoreDestroying, export ActionsCallback type

9c532f5

ai reviewed Aug 22, 2023

View reviewed changes

handle error in onActions

1716d8a

VladBrok added 4 commits August 23, 2023 18:49

improve onActions description

e97703f

add 1 more test for onActions

4ac2020

remove ignoreDestroying, move related logic to logux server

7b07c89

use different code example for onActions

97e0b6a

fix typo

8c0b40b

call inMap and inFilter outside of onActions

1b5fe54

ai reviewed Aug 26, 2023

View reviewed changes

Comment thread sync/index.js Outdated

review docs issue

aa99cbb

ai reviewed Aug 29, 2023

View reviewed changes

Comment thread base-node/index.d.ts Outdated

ai reviewed Aug 29, 2023

View reviewed changes

Comment thread base-node/index.d.ts

ai reviewed Aug 29, 2023

View reviewed changes

VladBrok and others added 2 commits August 29, 2023 23:47

Update base-node/index.d.ts

17ad47c

Co-authored-by: Andrey Sitnik <andrey@sitnik.ru>

Update base-node/index.d.ts

eccba39

Co-authored-by: Andrey Sitnik <andrey@sitnik.ru>

ai changed the base branch from main to next August 29, 2023 20:58

rename onActions to onSync, improve onSync tests

6b9dde1

VladBrok changed the title ~~Add onActions callback for implementing action queues~~ Add onSync callback for implementing action queues Aug 29, 2023

ai reviewed Aug 29, 2023

View reviewed changes

add inMap and inFilter to the onSync test

75b5057

ai approved these changes Aug 29, 2023

View reviewed changes

change onSync api to support calling access inside of queue

2137a4a

ai reviewed Sep 1, 2023

View reviewed changes

rename onSync to onReceive

bf66502

ai merged commit 8cb9d03 into logux:next Sep 10, 2023

	* Use it if you want more control over when an action will be added to the log
	*
	* Use it if you want more control over when an action will be added to the log.

Conversation

VladBrok commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ai Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ai commented Aug 22, 2023

Uh oh!

VladBrok commented Aug 23, 2023

Uh oh!

ai commented Aug 23, 2023

Uh oh!

VladBrok commented Aug 23, 2023

Uh oh!

ai commented Aug 23, 2023

Uh oh!

ai commented Aug 23, 2023

Uh oh!

VladBrok commented Aug 23, 2023

Uh oh!

ai commented Aug 23, 2023

Uh oh!

ai commented Aug 23, 2023

Uh oh!

VladBrok commented Aug 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VladBrok commented Aug 31, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VladBrok commented Aug 4, 2023 •

edited

Loading

ai Aug 22, 2023 •

edited

Loading