Introduce "shared" sessions #139

LeonidVas · 2020-11-08T17:01:38Z

Before the patchset a connection to server was synonym of the queue session. Now the session has a unique UUID (returned by the "queue.identify()" method), and one session can have many connections. To connect to an existing session, call "queue.identify(uuid)" with the previously obtained UUID.
Also, for the queue the ttr option has been added.
ttr in seconds - the time after which, if there is no active connection in the session, it will be released with all its tasks.

Closes #85

@ChangeLog:
The "shared" sessions was added to the queue. Previously, a connection to server was synonym of the queue session. Now the session has a unique UUID (returned by the "queue.identify()" method), and one session can have many connections. To connect to an existing session, call "queue.identify(uuid)" with the previously obtained UUID.
Also, for the queue the ttr option has been added.
ttr in seconds - the time after which, if there is no active connection in the session, it will be released with all its tasks.

queue/abstract.lua

README.md

queue/abstract.lua

LeonidVas · 2020-11-25T15:36:52Z

The benchmark was run before and after adding "shared" sessions.
The command that was used to run the test: rm -rf *.snap *.xlog && taskset 3 tarantool t/benchmark/multi_consumer_work.lua
For each variant of the parameters, ten runs of tests were carried out, and after the result was averaged.
Result format: (time it takes to fill the queue / time it takes to confirm the tasks) in microseconds.

Master:

consumers batches	1	100	10000
1	102 / 393	5479 / 10681	65914 / 322383
100	7272 / 9107	49043 / 224925	4962018 / 33961217
10000	41834 / 220479	3972183 / 25542455

With "shared" sessions:

consumers batches	1	100	10000
1	123 / 446	2776 / 12724	64437 / 509038
100	7900 / 8411	55467 / 274383	4899408 / 35677578
10000	54831 / 266131	4259797 / 30703081

In most cases, we have a loss of performance ~ 5 - 20%.

LeonidVas · 2020-11-30T21:40:58Z

After refactoring.

Master:

consumers batches	200	400
200	132887 / 659421	268149 / 1490263
400	247721 / 1538678	480667 / 3362063

With "shared" sessions:

consumers batches	200	400
200	136281 / 620819	259882 / 1426101
400	249045 / 1361905	490443 / 2613279

olegrok

Thanks for your patch. Consider several comments below. I'll give more detailed review a bit later.

Also I think we could push d8de8b9 out of order to simplify patchset and reduce diff.

README.md

queue/abstract/queue_session.lua

queue/abstract.lua

queue/abstract/queue_session.lua

README.md

queue/abstract.lua

queue/abstract/queue_session.lua

queue/abstract.lua

LeonidVas · 2020-12-16T11:59:48Z

Thanks for your patch. Consider several comments below. I'll give more detailed review a bit later.

Hi! Thanks for the review.

Also I think we could push d8de8b9 out of order to simplify patchset and reduce diff.

No, it's needed fo a3d6f4e

queue/abstract.lua

queue/abstract/queue_session.lua

Totktonada · 2020-12-17T22:51:37Z

queue/abstract.lua

@@ -7,7 +7,7 @@ local qc       = require('queue.compat')
 local num_type = qc.num_type
 local str_type = qc.str_type

-local session = box.session
+local connection = box.session


It looks as a 'connection' module that is a bit strange: we have no such server-side entity. How about defining a function to obtain a connection id, like so: local connection_id = box.session.id? Not sure here.

Additionally I use connection.on_disconnect()

Totktonada · 2020-12-17T22:53:54Z

queue/abstract/queue_session.lua

+
+-- methods
+local method = {
+    get_uuid = get_uuid,


We usually omit get_ for getters. How about connection_uuid()? It'll also more descriptive and more uniform if we'll rename remove_connection() to connection_remove() (so the connection_ will become a prefix).

What also disquets me a bit: there is no add_connection() or connection_add(), but there are get_uuid() and identify(). Maybe I'm too tired, but it looks hard to understand the difference between the two latter functions.

Not connection, session.
I think this question will be best to discussed by voice.

Totktonada · 2020-12-17T22:54:46Z

queue/abstract/queue_session.lua

+    on_session_remove = on_session_remove,
+    remove_connection = remove_connection,
+    set_settings = set_settings,
+    session_grant = session_grant,


It gives the ability to magane sessions from given user, right? How about just grant()?

Totktonada · 2020-12-17T22:58:15Z

queue/abstract.lua

@@ -640,6 +664,13 @@ local function build_stats(space)
    return stats
 end

+--- identify the session.


This description is a bit confusing for me. I would say something like 'Associate a current connection with given session'.

Totktonada · 2020-12-17T23:00:27Z

queue/abstract.lua

    -- wakeup all waiters
    while true do
-        waiter = box.space._queue_consumers.index.pk:min{id}
+        local waiter = box.space._queue_consumers.index.pk:min{conn_id}


Just to understand the idea: consumers are still identified by a connection id rather than queue session id? Can you explain, why it is different from identifying a task ownership?

Several consumers can work in the same session with the same tasks. For example, one consumer can take a task and another consumer (in the same session) can ack this task.

Totktonada · 2020-12-17T23:04:03Z

README.md

+In the queue the session has a unique UUID and one session can have many
+connections. Also, the consumer can reconnect to the existing session during the


one session can have many connections

I would say 'many connections may share one logical session' or kinda. It is a thing of taste, feel free to ignore.

Totktonada · 2020-12-17T23:04:43Z

README.md

+without parameters.  
+To connect to the existing session, call the `queue.identify(session_uuid)`
+with the UUID of the session.  
+In case of failure, an error will be thrown.


Just curious: what kinds of failures may appear here?

For example: Trying to use invalid format UUID, expired UUID.

Totktonada · 2020-12-17T23:27:26Z

README.md

+without parameters.  
+To connect to the existing session, call the `queue.identify(session_uuid)`
+with the UUID of the session.  
+In case of failure, an error will be thrown.


I would add an example for net.box to better describe the idea. I guess we can use on_connect net.box trigger to transparently overcome a reconnect.

As I said earlier, the examples will be added after the feature will be pushed to master.

Totktonada · 2020-12-17T23:33:36Z

README.md

@@ -372,6 +373,20 @@ Available `options`:
 * `ttr` - time to release in seconds. The time after which, if there is no active
 connection in the session, it will be released with all its tasks.

+## Session identify
+


I would highlight the ability we want to provide with this function somewhere at the start of the section, For example:

Sometimes we need an ability to acknowledge a task after reconnect (because retrying it is undesirable) or even acknowlegde using another connection.

Totktonada · 2020-12-17T23:36:56Z

queue/abstract/queue_session.lua

+    local queue_session_ids = box.space._queue_session_ids
+    local session_uuid = queue_session.get_uuid(conn_id)
+
+    queue_session_ids:delete{conn_id}


Jusr raw idea (maybe for further PRs): it may be helpful to have ability to trace connections and sessions and issue a log entry (on verbose or even debug level) when a connection is added or removed from a session or moved to another session.

Sounds good.

Totktonada · 2020-12-17T23:49:27Z

README.md

@@ -372,6 +373,20 @@ Available `options`:
 * `ttr` - time to release in seconds. The time after which, if there is no active
 connection in the session, it will be released with all its tasks.

+## Session identify


I'm not sure, but maybe it worth to name the section 'Overcome a reconnect'. That is what a user will actually find.

'Overcome a reconnect' is just one of the variants what for the sessions can be used.

Totktonada

I didn't verified the implementation thoroughly, but the idea looks okay for me.

I left some comments: mostly regarding API and presenting the feature in the readme. Some are just to better understand the idea.

The term "queue session" will be added to the queue while working on #85. One "queue session" will include many connections (box.session). For clarity, box.session has been renamed to connection. Needed for #85

Needed for #85

In the future, we will refuse to use the "pk" index of the "_queue_taken" space to improve performance (the conclusion about the improvement in performance is based on the benchmarks that will be added in one of the next commits). So, let's minimize its use. Needed for #85

The same time functions are used in several files, so move them to a separate file. Needed for #85

Needed for #85

The patch adds the ability to set the queue settings(by calling "queue.cfg(opts)"). Now only one setting is available - "ttr"(time to release). "ttr" in seconds - the time after which, if there is no active connection in the session, it will be released with all its tasks. Part of #85

Before the patch a connection to server was synonym of the queue session. Now the session has a unique UUID (returned by the "queue.identify()" method), and one session can have many connections. The session will be deleted (all session tasks will be released) after "ttr" seconds have passed since the last connection was disconnected. To connect to an existing session, call "queue.identificate(uuid)" with the previously obtained UUID. "ttr" in seconds - the time after which, if there is no active connection in the session, it will be released with all its tasks. Also, the "_queue_taken" internal space has been replaced with the "_queue_taken_2" with a change format and used indexes (for better performance). The downgrade to previous version works correctly. Closes of #85

Related to #105 Follows up #85

Follows up #85

LeonidVas requested review from Totktonada and Mons November 8, 2020 17:02

LeonidVas self-assigned this Nov 8, 2020

LeonidVas commented Nov 10, 2020

View reviewed changes

queue/abstract.lua Outdated Show resolved Hide resolved

Totktonada reviewed Nov 16, 2020

View reviewed changes

queue/abstract.lua Outdated Show resolved Hide resolved

Totktonada reviewed Nov 16, 2020

View reviewed changes

queue/abstract.lua Outdated Show resolved Hide resolved

Totktonada reviewed Nov 16, 2020

View reviewed changes

queue/abstract.lua Outdated Show resolved Hide resolved

Totktonada reviewed Nov 16, 2020

View reviewed changes

queue/abstract.lua Outdated Show resolved Hide resolved

Totktonada reviewed Nov 17, 2020

View reviewed changes

README.md Show resolved Hide resolved

Totktonada reviewed Nov 17, 2020

View reviewed changes

queue/abstract.lua Outdated Show resolved Hide resolved

Totktonada reviewed Nov 17, 2020

View reviewed changes

queue/abstract.lua Outdated Show resolved Hide resolved

RunsFor reviewed Nov 23, 2020

View reviewed changes

queue/abstract.lua Outdated Show resolved Hide resolved

LeonidVas force-pushed the lvasiliev/gh-85-add-possibility-work-after-reconnect branch from 9b89fe4 to 1d8f708 Compare November 25, 2020 14:41

LeonidVas force-pushed the lvasiliev/gh-85-add-possibility-work-after-reconnect branch from 1d8f708 to d0b2b21 Compare November 30, 2020 20:28

LeonidVas force-pushed the lvasiliev/gh-85-add-possibility-work-after-reconnect branch from d0b2b21 to c94c30b Compare November 30, 2020 21:57

LeonidVas requested review from RunsFor and Totktonada November 30, 2020 21:58

LeonidVas requested a review from olegrok December 14, 2020 13:45

olegrok reviewed Dec 14, 2020

View reviewed changes

olegrok reviewed Dec 15, 2020

View reviewed changes

queue/abstract.lua Outdated Show resolved Hide resolved

olegrok reviewed Dec 15, 2020

View reviewed changes

queue/abstract.lua Outdated Show resolved Hide resolved

olegrok reviewed Dec 15, 2020

View reviewed changes

queue/abstract.lua Show resolved Hide resolved

LeonidVas force-pushed the lvasiliev/gh-85-add-possibility-work-after-reconnect branch 2 times, most recently from 9c8bf69 to bf1faf5 Compare December 16, 2020 14:31

LeonidVas requested a review from olegrok December 16, 2020 14:34

olegrok reviewed Dec 16, 2020

View reviewed changes

queue/abstract.lua Show resolved Hide resolved

olegrok reviewed Dec 16, 2020

View reviewed changes

queue/abstract/queue_session.lua Show resolved Hide resolved

Totktonada reviewed Dec 17, 2020

View reviewed changes

Totktonada approved these changes Dec 17, 2020

View reviewed changes

This was referenced Dec 21, 2020

Track performance of tarantool/queue tarantool/bench-run#16

Open

Add microb coverage #29

Closed

LeonidVas added 7 commits December 22, 2020 17:36

refactoring: rename session to connection

19e839e

The term "queue session" will be added to the queue while working on #85. One "queue session" will include many connections (box.session). For clarity, box.session has been renamed to connection. Needed for #85

refactoring: move task check to "taken" into a separate function

b2481f7

Needed for #85

refactoring: move time functions to util

3fa50b3

The same time functions are used in several files, so move them to a separate file. Needed for #85

util: add a description of time functions

c7cddc6

refactoring: move "release session tasks" to a separate function

c639351

Needed for #85

LeonidVas force-pushed the lvasiliev/gh-85-add-possibility-work-after-reconnect branch from ac664e5 to 373139c Compare December 22, 2020 14:36

LeonidVas added 4 commits December 22, 2020 17:51

benchmark: add a benchmark for working with multiple consumers

b2c1874

Related to #105 Follows up #85

documentation: add description of the queue settings

2d0af50

Follows up #85

documentation: add description of the session identification

00365d6

Follows up #85

LeonidVas force-pushed the lvasiliev/gh-85-add-possibility-work-after-reconnect branch from 373139c to 00365d6 Compare December 22, 2020 14:52

LeonidVas merged commit 8a8818f into master Dec 24, 2020

LeonidVas deleted the lvasiliev/gh-85-add-possibility-work-after-reconnect branch December 24, 2020 18:41

oleg-jukovec mentioned this pull request Aug 16, 2022

Add note to README.md: queue.identify() returns string instead of uuid type #182

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce "shared" sessions #139

Introduce "shared" sessions #139

LeonidVas commented Nov 8, 2020 •

edited

Loading

LeonidVas commented Nov 25, 2020 •

edited

Loading

LeonidVas commented Nov 30, 2020

olegrok left a comment

LeonidVas commented Dec 16, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada Dec 17, 2020

LeonidVas Dec 18, 2020

Totktonada left a comment

		In the queue the session has a unique UUID and one session can have many
		connections. Also, the consumer can reconnect to the existing session during the

Introduce "shared" sessions #139

Introduce "shared" sessions #139

Conversation

LeonidVas commented Nov 8, 2020 • edited Loading

LeonidVas commented Nov 25, 2020 • edited Loading

LeonidVas commented Nov 30, 2020

olegrok left a comment

Choose a reason for hiding this comment

LeonidVas commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Totktonada left a comment

Choose a reason for hiding this comment

LeonidVas commented Nov 8, 2020 •

edited

Loading

LeonidVas commented Nov 25, 2020 •

edited

Loading