feat(ds): list disconnected persistent sessions in clients API #12500

thalesmg · 2024-02-09T20:02:27Z

Fixes https://emqx.atlassian.net/browse/EMQX-11540

Note that not all information provided by disconnected in-memory sessions is available to
disconnected persistent sessions, nor does all of them make sense.

Release version: v/e5.7

Summary

PR Checklist

Please convert it to a draft if any of the following conditions are not met. Reviewers may skip over until all the items are checked:

Added tests for the changes
Added property-based tests for code which performs user input validation
Changed lines covered in coverage report
Change log has been added to changes/(ce|ee)/(feat|perf|fix|breaking)-<PR-id>.en.md files
For internal contributor: there is a jira ticket to track this change
Created PR to emqx-docs if documentation update is required, or link to a follow-up jira ticket
Schema changes are backward compatible

Checklist for CI (.github/workflows) changes

If changed package build workflow, pass this action (manual trigger)
Change log has been added to changes/ dir for user-facing artifacts update

ieQu1 · 2024-02-11T00:37:24Z

apps/emqx_management/src/emqx_mgmt.erl

+%% Internal exports
+%%--------------------------------------------------------------------
+
+lookup_clientid_ets(ClientId, FormatFun) ->


Rename to lookup_running_client ?

keynslug

👍🏼

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

apps/emqx_management/src/emqx_mgmt_api_clients.erl

keynslug · 2024-02-14T16:51:46Z

apps/emqx_management/src/emqx_mgmt_api_clients.erl

+no_persistent_sessions() ->
+    case emqx_persistent_message:is_persistence_enabled() of
+        true ->
+            emqx_persistent_session_ds_state:is_end_of_table(init_persistent_session_iterator());


Q: Why use separate function here while there's a simple comparison with $end_of_table down below?

Good question. In my first attempt, it was just like that. But dialyzer complained about such a comparison here (because the iterator is opaque), so I had to introduce this function...

Curiously enough, it's not complaining about the use case in the function guard, in this same file. 🤔

Maybe we could make that typespec:

-opaque session_iterator_inner() :: emqx_persistent_session_ds:id(). -type session_iterator() :: session_iterator_inner() | '$end_of_table'.

This spec feels a bit odd to me: hard to make sense of a union of opaque type and non-opaque type. I think it's better either to use is_end_of_table/1 everywhere, to turn it into a regular type, or to change iteration function's contract into something like next(iterator()) -> {_Items, {more, iterator()} | fin}.

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

ieQu1 · 2024-02-20T15:31:23Z

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

+
+            ok


Nit: not needed?

Those oks are just for ease of adding new lines without adding commas to the diff. e.g.: ?assert(...). would become ?assert(...), just to add a newline.

I think those oks are innocuous, hence I prefer to leave them there.

Not sure if we have to optimize code for "minimal diffs" metric. I would prefer to optimize for "readability at any given moment of time".

Sure, no need to optimize. Still, in my opinion, this extra line does not hinder readability. 🤔

ieQu1 · 2024-02-20T15:31:40Z

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

+
+            ok


Nit: not needed?

Those oks are just for ease of adding new lines without adding commas to the diff. e.g.: ?assert(...). would become ?assert(...), just to add a newline.

I think those oks are innocuous, hence I prefer to leave them there.

ieQu1 · 2024-02-20T15:32:07Z

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

+
+            ok


Those oks are just for ease of adding new lines without adding commas to the diff. e.g.: ?assert(...). would become ?assert(...), just to add a newline.

I think those oks are innocuous, hence I prefer to leave them there.

ieQu1 · 2024-02-20T15:39:02Z

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

+        {error, _} -> X
+    end.
+
+list_request(Port) ->


Is it possible to get IP and port from the CT Config variable or emqx config?

Could be, I think it's a matter of style/choice.

ieQu1 · 2024-02-20T15:42:47Z

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

+
+t_persistent_sessions5(Config) ->
+    [N1, N2] = ?config(nodes, Config),
+    APIPort = 18084,


Why is it using non-standard port?

Because the suite starts the management on the default port for the other tests.

And why can't we reuse the default setup?
Also, is it possible to call the Erlang interface directly, bypassing conversion from/to JSON and the HTTP?

And why can't we reuse the default setup?

Those tests are using an isolated, clustered setup. To avoid having to stop the running applications from the suite, it's simpler to just set another port here.

Also, is it possible to call the Erlang interface directly, bypassing conversion from/to JSON and the HTTP?

I believe it's best to test the user-facing API in this case, since it's the object of the test. Specially during the bridge refactoring tasks, we observed that tests that don't use such APIs tend to hinder future changes to the code.

apps/emqx_management/test/emqx_mgmt_api_clients_SUITE.erl

keynslug · 2024-02-20T16:23:56Z

apps/emqx/src/emqx_persistent_session_ds_state.erl

+    %% N.B.: this is potentially costly.  Should not be called in hot paths.
+    %% `mnesia:table_info(_, size)' is always zero for rocksdb, so we need to traverse...
+    do_session_count(make_session_iterator(), 0).
+
 -spec make_session_iterator() -> session_iterator().
 make_session_iterator() ->
    case mnesia:dirty_first(?session_tab) of


Q: What this case clause is for?

Good question. I didn't introduce it, but seems like we could "eta reduce" it. 🤔

keynslug · 2024-02-20T16:24:38Z

apps/emqx/src/emqx_persistent_session_ds_state.erl

+-opaque session_iterator_inner() :: emqx_persistent_session_ds:id().
+-type session_iterator() :: session_iterator_inner() | '$end_of_table'.


At first glance it honestly looks essentially equivalent to -type session_iterator() :: emqx_persistent_session_ds:id() | '$end_of_table'..

ieQu1 · 2024-02-20T16:50:45Z

apps/emqx/src/emqx_persistent_session_ds_state.erl

@@ -53,7 +59,7 @@

 -type subscriptions() :: emqx_topic_gbt:t(_SubId, emqx_persistent_session_ds:subscription()).

-opaque session_iterator() :: emqx_persistent_session_ds:id() | '$end_of_table'.
+-type session_iterator() :: emqx_persistent_session_ds:id() | '$end_of_table'.


What triggered the change from opaque to transparent? Does the new code rely on the internal structure of the iterator? Ideally, thatt should be avoided, because I am planning to change the storage from mnesia to DS in nearest future, so it would be helpful to contain the algorithm for iteration in one place.

What triggered this was the check for no persistent sessions, and also if the iteration has reached the end of the table. Basically, dialyzer complained about equality comparison with $end_of_table.

P.S. I think the return type of session_interator_next can be changed like this:

-spec session_iterator_next(session_iterator(), pos_integer()) -> {[{emqx_persistent_session_ds:id(), metadata()}], session_iterator() | '$end_of_table'}.

...Hopefully it will satisfy dialyzer.

Good idea, I'll adjust it.

Actually, checking for an empty table won't work with this, but I'll try another approach.

This check is just comparing the first batch with [] and batch size = 1? I think this can be done in the mgmt code to keep the interface of the CRUD module lean and clean.

ieQu1 · 2024-02-20T17:21:38Z

apps/emqx/src/emqx_persistent_session_ds_state.erl

+do_session_count('$end_of_table', N) ->
+    N;
+do_session_count(Cursor, N) ->
+    do_session_count(mnesia:dirty_next(?session_tab, Cursor), N + 1).


Can be implemented via session_iterator_next instead of mnesia:dirty_next()? The parent function already uses make_session_iterator()...

Fixes https://emqx.atlassian.net/browse/EMQX-11540 Note that not all information provided by disconnected in-memory sessions is available to disconnected persistent sessions, nor does all of them make sense.

ieQu1 · 2024-02-20T22:41:09Z

apps/emqx/src/emqx_persistent_session_ds_state.erl

@@ -359,17 +369,18 @@ del_rank(Key, Rec) ->
 fold_ranks(Fun, Acc, Rec) ->
    gen_fold(ranks, Fun, Acc, Rec).

+-spec session_count() -> non_neg_integer().


Ideally, I would prefer to move this function to mgmt. This module was designed to encapsulate access to the session states, completely devoid of the business logic, rather than a convenient place for the helper functions.

If we expose a session count function from this module, to me it means that we commit ourselves to providing session count to business logic. But since the current implementation requires a full sweep of the table, perhaps it's best to avoid it for now.

Good point. I'll fix this in a follow up.

Follow up to emqx#12500 (comment)

test: add facility for starting emqx_dashboard with emqx_cth_suite

0515c55

thalesmg force-pushed the ds-list-client-api-m-20240209 branch 2 times, most recently from eb2703e to 3615cec Compare February 9, 2024 20:35

thalesmg marked this pull request as ready for review February 9, 2024 21:03

thalesmg requested review from lafirest and a team as code owners February 9, 2024 21:03

ieQu1 reviewed Feb 11, 2024

View reviewed changes

keynslug reviewed Feb 14, 2024

View reviewed changes

thalesmg force-pushed the ds-list-client-api-m-20240209 branch 2 times, most recently from 17fea76 to c1a7180 Compare February 19, 2024 17:36

ieQu1 reviewed Feb 20, 2024

View reviewed changes

thalesmg force-pushed the ds-list-client-api-m-20240209 branch from c1a7180 to 4206931 Compare February 20, 2024 15:59

keynslug previously approved these changes Feb 20, 2024

View reviewed changes

thalesmg dismissed keynslug’s stale review via 88a0792 February 20, 2024 16:24

thalesmg force-pushed the ds-list-client-api-m-20240209 branch 3 times, most recently from e161491 to 3db3fd6 Compare February 20, 2024 16:47

ieQu1 reviewed Feb 20, 2024

View reviewed changes

thalesmg force-pushed the ds-list-client-api-m-20240209 branch from 3db3fd6 to 8078684 Compare February 20, 2024 17:16

ieQu1 reviewed Feb 20, 2024

View reviewed changes

thalesmg force-pushed the ds-list-client-api-m-20240209 branch from 8078684 to 977690a Compare February 20, 2024 17:31

feat(ds): list disconnected persistent sessions in clients API

3a4c7f6

Fixes https://emqx.atlassian.net/browse/EMQX-11540 Note that not all information provided by disconnected in-memory sessions is available to disconnected persistent sessions, nor does all of them make sense.

thalesmg force-pushed the ds-list-client-api-m-20240209 branch from 977690a to 3a4c7f6 Compare February 20, 2024 17:53

ieQu1 approved these changes Feb 20, 2024

View reviewed changes

ieQu1 reviewed Feb 20, 2024

View reviewed changes

thalesmg merged commit 529211b into emqx:master Feb 21, 2024
166 checks passed

thalesmg deleted the ds-list-client-api-m-20240209 branch February 21, 2024 12:02

thalesmg added a commit to thalesmg/emqx that referenced this pull request Feb 21, 2024

refactor(mgmt): move persistent session counting function to mgmt module

4522f1e

Follow up to emqx#12500 (comment)

thalesmg mentioned this pull request Feb 21, 2024

refactor(mgmt): move persistent session counting function to mgmt module #12555

Merged

9 tasks

thalesmg added a commit to thalesmg/emqx that referenced this pull request Feb 21, 2024

refactor(mgmt): move persistent session counting function to mgmt module

03138f8

Follow up to emqx#12500 (comment)

BrewTestBot mentioned this pull request Mar 28, 2024

emqx 5.6.0 Homebrew/homebrew-core#167392

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ds): list disconnected persistent sessions in clients API #12500

feat(ds): list disconnected persistent sessions in clients API #12500

thalesmg commented Feb 9, 2024 •

edited

ieQu1 Feb 11, 2024

keynslug left a comment

keynslug Feb 14, 2024

thalesmg Feb 19, 2024

thalesmg Feb 19, 2024

keynslug Feb 20, 2024

ieQu1 Feb 20, 2024

thalesmg Feb 20, 2024

ieQu1 Feb 20, 2024 •

edited

thalesmg Feb 20, 2024

ieQu1 Feb 20, 2024

thalesmg Feb 20, 2024 •

edited

ieQu1 Feb 20, 2024

thalesmg Feb 20, 2024

ieQu1 Feb 20, 2024

thalesmg Feb 20, 2024

ieQu1 Feb 20, 2024

thalesmg Feb 20, 2024

ieQu1 Feb 20, 2024 •

edited

thalesmg Feb 20, 2024

keynslug Feb 20, 2024

thalesmg Feb 20, 2024

keynslug Feb 20, 2024

ieQu1 Feb 20, 2024 •

edited

thalesmg Feb 20, 2024 •

edited

ieQu1 Feb 20, 2024 •

edited

thalesmg Feb 20, 2024

thalesmg Feb 20, 2024

ieQu1 Feb 20, 2024 •

edited

ieQu1 Feb 20, 2024 •

edited

ieQu1 Feb 20, 2024 •

edited

thalesmg Feb 21, 2024

thalesmg Feb 21, 2024

		-opaque session_iterator_inner() :: emqx_persistent_session_ds:id().
		-type session_iterator() :: session_iterator_inner() \| '$end_of_table'.

feat(ds): list disconnected persistent sessions in clients API #12500

feat(ds): list disconnected persistent sessions in clients API #12500

Conversation

thalesmg commented Feb 9, 2024 • edited

Summary

PR Checklist

Checklist for CI (.github/workflows) changes

Choose a reason for hiding this comment

keynslug left a 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

ieQu1 Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thalesmg Feb 20, 2024 • edited

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

ieQu1 Feb 20, 2024 • edited

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

ieQu1 Feb 20, 2024 • edited

Choose a reason for hiding this comment

thalesmg Feb 20, 2024 • edited

Choose a reason for hiding this comment

ieQu1 Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ieQu1 Feb 20, 2024 • edited

Choose a reason for hiding this comment

ieQu1 Feb 20, 2024 • edited

Choose a reason for hiding this comment

ieQu1 Feb 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thalesmg commented Feb 9, 2024 •

edited

ieQu1 Feb 20, 2024 •

edited

thalesmg Feb 20, 2024 •

edited

ieQu1 Feb 20, 2024 •

edited

ieQu1 Feb 20, 2024 •

edited

thalesmg Feb 20, 2024 •

edited

ieQu1 Feb 20, 2024 •

edited

ieQu1 Feb 20, 2024 •

edited

ieQu1 Feb 20, 2024 •

edited

ieQu1 Feb 20, 2024 •

edited