Service for database domain management #3052

arcusfelis · 2021-03-08T07:55:04Z

Proposed changes include:

Service
Core
DB queries
Docs

DONE:

We need to write a gen_server, which removes old records from the domain_events table.
Pagination tests - many records inserted. Maybe proper tests.
Other DB backends, not just Postgres.
Distributed tests - more than one node (though do we really need it? Other nodes would read events from the domain_events table in exactly the same way, as one node does).
- Need to add tests to check what is happening when service goes down.

src/domain/mongoose_domain_sql.erl

chrzaszcz · 2021-03-08T08:08:47Z

Please describe the architecture, e.g. why two database tables are needed. One commit with 1000 lines of code and no description of the changes makes it difficult to figure out the reasoning behind all the changes.

For PR's still in progress (not intended for review and merging yet) please use the 'Draft' option on GitHub.

codecov · 2021-03-08T08:14:42Z

Codecov Report

Merging #3052 (874999d) into multi-tenancy-phase-1 (135fec2) will increase coverage by 0.06%.
The diff coverage is 84.32%.

@@                    Coverage Diff                    @@
##           multi-tenancy-phase-1    #3052      +/-   ##
=========================================================
+ Coverage                  78.53%   78.60%   +0.06%     
=========================================================
  Files                        378      385       +7     
  Lines                      31119    31438     +319     
=========================================================
+ Hits                       24440    24712     +272     
- Misses                      6679     6726      +47

Impacted Files	Coverage Δ
src/config/mongoose_config_spec.erl	`98.80% <ø> (ø)`
src/domain/mongoose_domain_utils.erl	`0.00% <0.00%> (ø)`
src/domain/mongoose_domain_db_cleaner.erl	`76.47% <76.47%> (ø)`
src/domain/service_domain_db.erl	`80.70% <80.70%> (ø)`
src/domain/mongoose_domain_core.erl	`82.85% <82.85%> (ø)`
src/domain/mongoose_domain_loader.erl	`82.85% <82.85%> (ø)`
src/domain/mongoose_domain_api.erl	`90.32% <90.32%> (ø)`
src/domain/mongoose_domain_sql.erl	`91.01% <91.01%> (ø)`
src/ejabberd_app.erl	`93.75% <100.00%> (+0.06%)`	⬆️
src/elasticsearch/mongoose_elasticsearch.erl	`76.92% <0.00%> (-7.70%)`	⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 135fec2...874999d. Read the comment docs.

src/domain/mongoose_domain_core.erl

Two new DB tables: domain_settings and domain_events. Added service service_domain_db. Added prepared queries in mongoose_domain_sql. The entry point for logic is in mongoose_domain_api. The module mongoose_domain_core should be always started. service_domain_db needs to be specified in the TOML config. service_domain_db_SUITE contains tests.

src/domain/mongoose_domain_loader.erl

src/domain/mongoose_domain_core.erl

DenysGonchar · 2021-03-10T17:30:27Z

src/domain/mongoose_domain_core.erl

+            false
+    end.
+
+remove_all_unlocked() ->


What is the purpose of such interface? There must be per domain operations, group removal should not be possible. Otherwise, it's impossible to implement domain add/remove hooks.

It is the simplest way to recover if gen_server of a DB service crashes.

we must rework this. if this ETS is a source of truth for other components, it cannot spontaneously lose all the domains.

putting here some notes after today's discussion:

there are 2 different cases when domains DB service can potentially crash:

During the MIM startup. before the initial filling of the ETS table (from DB) is done.

after initial ETS filling.

for the first case it's acceptable to completely drop the core's ETS table, because we cannot use events DB table to perform an update of the ETS table with the latest changes.

for the second case, we must use events DB table.

implementation notes:

core's gen_server must store the last applied event id.

at the beginning, before the initial ETS filling, this event id is undefined.

once initial ETS filling is done, event id is set for the first time.

after that event id is updated with each and every change of the ETS.

it must be possible to drop ETS data only if event id is not set yet.

after initialization of the event id, DB service (on restart) must use that stored event id to apply the latest changes to the ETS.

also we may want to extend the cleaning interval and max age parameters for the events DB table to some higher value, e.g. 1 day

@arcusfelis please add some implementation note in the source code.

src/domain/mongoose_domain_core.erl

DenysGonchar · 2021-03-10T19:45:14Z

src/domain/mongoose_domain_api.erl

+%% must be added to the core MIM component described above.
+%% Domains should be nameprepped using `jid:nameprep'
+-spec init([pair()]) -> ok | {error, term()}.
+init(Pairs) ->


It must accept as a second parameter the list of allowed host types.
Only operation with the specified host types are allowed! if an invalid host type is used, the corresponding error must be returned.

Also host types are just strings. So every correct domain name is a correct host type identifier, but not every host type is a correct domain name

Also host types are just strings. So every correct domain name is a correct host type identifier, but not every host type is a correct domain name

so, it would not make it interchangeable with any code, which does nameprepping for the Host argument. Or requires host-type in the specification (hook api-s, pool api-s).

the host types automatically created for hosts declaration in config file will be interchangeable.

but the final goal is to ensure that MIM supports any string id for the host_type. that is one of the sanity checks.

src/domain/mongoose_domain_api.erl

Add db_reinserted_from_one_node_while_service_disabled_on_another testcase

chrzaszcz

Lots of good work! I have some comments, but with such a big change it is inevitable.

big_tests/tests/service_domain_db_SUITE.erl

src/domain/mongoose_domain_core.erl

big_tests/tests/service_domain_db_SUITE.erl

src/domain/service_domain_db.erl

src/domain/mongoose_domain_sql.erl

src/domain/mongoose_domain_api.erl

src/domain/service_domain_db.erl

…n_management.md

DenysGonchar · 2021-03-16T17:02:13Z

src/domain/mongoose_domain_utils.erl

+-module(mongoose_domain_utils).
+-export([halt_node/1]).
+
+halt_node(ExitText) ->


Just a side note, no need to change anything:

we should think about one common MIM helper module for such functionality

DenysGonchar

just a side note, no need to change anything in this PR:

in the future we should split service_domain_db_SUITE.erl into 2 separate suites:

one for the domains core module testing (MB even make it a unit test)
another for service testing

and of course we will have to extend (and rework if required) dynamic domain documentation as we progress with implementation.

chrzaszcz

Minor comments that should be addressed in subsequent PR's.

chrzaszcz · 2021-03-17T07:20:02Z

big_tests/tests/service_domain_db_SUITE.erl

+     db_reanabled_domain_is_in_db,
+     db_reanabled_domain_is_in_core,


Minor typo: reenabled

src/domain/mongoose_domain_api.erl

chrzaszcz · 2021-03-17T07:28:28Z

src/domain/mongoose_domain_api.erl

+get_all_static() ->
+    mongoose_domain_core:get_all_static().
+
+%% Get the list of the host\_types provided during initialisation


Minor:

Suggested change

%% Get the list of the host\_types provided during initialisation

%% Get the list of the host_types provided during initialisation

chrzaszcz · 2021-03-17T07:38:33Z

src/domain/mongoose_domain_loader.erl

+    end.
+
+check_for_updates(FromId, PageSize) ->
+    check_if_from_num_still_relevant(FromId),


Minor:

Suggested change

check_if_from_num_still_relevant(FromId),

check_if_from_id_still_relevant(FromId),

chrzaszcz · 2021-03-17T07:39:09Z

src/domain/mongoose_domain_loader.erl

+
+check_for_updates(FromId, PageSize) ->
+    check_if_from_num_still_relevant(FromId),
+    %% Ordered by the earlist events first


Minor:

Suggested change

%% Ordered by the earlist events first

%% Ordered by the earliest events first

src/domain/mongoose_domain_loader.erl

chrzaszcz · 2021-03-17T07:44:20Z

src/domain/mongoose_domain_loader.erl

+check_if_from_num_still_relevant(0) ->
+    ok;


This returns ok but what if some records were added in the meantime (while there was no connection to the DB) and then cleanup happened? I think that we should still check that Min = 1 or something like that to make sure no cleanup was done.

arcusfelis commented Mar 8, 2021

View reviewed changes

src/domain/mongoose_domain_sql.erl Outdated Show resolved Hide resolved

arcusfelis force-pushed the mu-multi-tenancy-phase-1-db branch from fbd21ff to 7978a7b Compare March 8, 2021 10:23

arcusfelis marked this pull request as draft March 8, 2021 10:27

arcusfelis commented Mar 8, 2021

View reviewed changes

src/domain/mongoose_domain_core.erl Outdated Show resolved Hide resolved

arcusfelis force-pushed the mu-multi-tenancy-phase-1-db branch from 7978a7b to 1cbf7cf Compare March 9, 2021 10:01

arcusfelis added 8 commits March 9, 2021 12:40

Add docs for service_domain_db

793fa38

Use helper functions from rdbms_queries for pagination

261b007

Add MySQL schema for the domain service

779e50b

Pass tests for all RDBMS databases

1d8dc88

Fix service_domain_db:config_spec/0 dialyzer warning

31b9a73

Add DB cleaner fo the domain events

8e5903a

Document event_cleaning_interval and event_max_age options

8fd9252

arcusfelis force-pushed the mu-multi-tenancy-phase-1-db branch from 36607d8 to 8fd9252 Compare March 9, 2021 11:40

Fix dialyzer warning for the null id case

07232c6

chrzaszcz reviewed Mar 9, 2021

View reviewed changes

src/domain/mongoose_domain_loader.erl Outdated Show resolved Hide resolved

arcusfelis added 3 commits March 9, 2021 14:34

Skip DB tests if RDBMS not available

60a670c

Add get_all_locked and get_domains_by_host_type functions

367a9de

Set read_concurrency=true

97f5251

arcusfelis requested review from DenysGonchar and chrzaszcz March 9, 2021 15:40

arcusfelis marked this pull request as ready for review March 9, 2021 15:40

DenysGonchar reviewed Mar 10, 2021

View reviewed changes

src/domain/mongoose_domain_core.erl Show resolved Hide resolved

DenysGonchar reviewed Mar 10, 2021

View reviewed changes

src/domain/mongoose_domain_core.erl Outdated Show resolved Hide resolved

DenysGonchar reviewed Mar 10, 2021

View reviewed changes

src/domain/mongoose_domain_core.erl Outdated Show resolved Hide resolved

DenysGonchar reviewed Mar 10, 2021

View reviewed changes

src/domain/mongoose_domain_api.erl Outdated Show resolved Hide resolved

Add db_inserted_from_one_node_while_service_disabled_on_another testcase

86c6223

Add db_reinserted_from_one_node_while_service_disabled_on_another testcase

chrzaszcz reviewed Mar 15, 2021

View reviewed changes

Rename locked into static

4fb9826

chrzaszcz reviewed Mar 16, 2021

View reviewed changes

src/domain/service_domain_db.erl Outdated Show resolved Hide resolved

arcusfelis added 17 commits March 16, 2021 10:32

Rename remove to delete

26954ef

Replace Removal with remove

2c676ba

Clean comments in mongoose_domain_api

96e88c5

Rename doc/modules/service_domain_db.md -> doc/developers-guide/domai…

803b5f7

…n_management.md

Change wording in the docs

588adba

Change the way service_domain_db:enabled/0 works

508ac8f

Added comment about cleaning event table happens from all nodes

c98b589

Rename FromNum to FromId

7e9cbf6

Use lists:foreach/2

b501a7f

Fix typo

ba50ce0

Change order of init in service_domain_db

b4e31a6

One init_per_testcase for all test cases

ea28020

Remove precond function

0effe88

Skip group if db not configured

12568e8

Add db_initial_load_crashes_node testcase

d2df37b

Add db_out_of_sync_crashes_node testcase

09563fe

Move db_initial_load_crashes_node into db group

0b4bd53

arcusfelis requested review from chrzaszcz and DenysGonchar March 16, 2021 16:03

DenysGonchar reviewed Mar 16, 2021

View reviewed changes

DenysGonchar approved these changes Mar 16, 2021

View reviewed changes

Receive all messages in receive_all_check_for_updates

874999d

chrzaszcz approved these changes Mar 17, 2021

View reviewed changes

chrzaszcz merged commit 3626394 into multi-tenancy-phase-1 Mar 17, 2021

chrzaszcz deleted the mu-multi-tenancy-phase-1-db branch March 17, 2021 07:54

DenysGonchar mentioned this pull request Apr 22, 2021

Multi tenancy phase 1 #3095

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service for database domain management #3052

Service for database domain management #3052

arcusfelis commented Mar 8, 2021 •

edited

Loading

chrzaszcz commented Mar 8, 2021 •

edited

Loading

codecov bot commented Mar 8, 2021 •

edited

Loading

DenysGonchar Mar 10, 2021

arcusfelis Mar 11, 2021

DenysGonchar Mar 11, 2021 •

edited

Loading

DenysGonchar Mar 12, 2021

DenysGonchar Mar 10, 2021 •

edited

Loading

arcusfelis Mar 11, 2021

DenysGonchar Mar 11, 2021 •

edited

Loading

chrzaszcz left a comment •

edited

Loading

DenysGonchar Mar 16, 2021

DenysGonchar left a comment •

edited

Loading

chrzaszcz left a comment

chrzaszcz Mar 17, 2021

chrzaszcz Mar 17, 2021

chrzaszcz Mar 17, 2021

chrzaszcz Mar 17, 2021

chrzaszcz Mar 17, 2021

		db_reanabled_domain_is_in_db,
		db_reanabled_domain_is_in_core,

	%% Get the list of the host\_types provided during initialisation
	%% Get the list of the host_types provided during initialisation

	check_if_from_num_still_relevant(FromId),
	check_if_from_id_still_relevant(FromId),

	%% Ordered by the earlist events first
	%% Ordered by the earliest events first

		check_if_from_num_still_relevant(0) ->
		ok;

Service for database domain management #3052

Service for database domain management #3052

Conversation

arcusfelis commented Mar 8, 2021 • edited Loading

chrzaszcz commented Mar 8, 2021 • edited Loading

codecov bot commented Mar 8, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenysGonchar Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenysGonchar Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenysGonchar Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

chrzaszcz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DenysGonchar left a comment • edited Loading

Choose a reason for hiding this comment

chrzaszcz 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

arcusfelis commented Mar 8, 2021 •

edited

Loading

chrzaszcz commented Mar 8, 2021 •

edited

Loading

codecov bot commented Mar 8, 2021 •

edited

Loading

DenysGonchar Mar 11, 2021 •

edited

Loading

DenysGonchar Mar 10, 2021 •

edited

Loading

DenysGonchar Mar 11, 2021 •

edited

Loading

chrzaszcz left a comment •

edited

Loading

DenysGonchar left a comment •

edited

Loading