Add initial code #1

arcusfelis · 2022-05-09T11:03:44Z

Check readme for description

Use persistent term for metadata Monitor a proxy process

Report time to get the joining lock

Register name for disco

Remove try-catch

Update readme

Separate operations for delete and delete_many Add more types

To split the business logic from the helpers logic

pawlooss1

Nice work 👍 I left some comments that I think could improve readability and maintainability.

pawlooss1 · 2023-02-27T10:53:20Z

src/cets_long.erl

+            case Catch of
+                true -> just_run_safely(Info#{what => long_task_failed}, Fun);
+                false -> Fun()
+            end
+        after
+            Diff = diff(Start),
+            ?LOG_INFO(Info#{what => long_task_finished, time_ms => Diff}),
+            Pid ! stop


This indent is unnecessary big. I see a similar case in some other places as well. What do you think about plugging in some code formatter, such as https://github.com/WhatsApp/erlfmt? I think it could help us keep the formatting consistent.

I have so many questions to erlfmt :)
At least for nodejs formatters you can configure the style.

That aligning when when is used is kinda wtf:

resolve(#amp_strategy{deliver = [Value | _]}, #amp_rule{ condition = deliver, value = Value, action = Action }) when Action == drop orelse Action == error -> match; resolve(#amp_strategy{'match-resource' = Value}, #amp_rule{ condition = 'match-resource', value = any }) when Value /= undefined -> match; resolve(#amp_strategy{'match-resource' = Value}, #amp_rule{ condition = 'match-resource', value = Value }) ->

And brackets:

CommandTags = ets:match( ejabberd_commands, #ejabberd_commands{ name = '$1', tags = '$2', _ = '_' } ),

Maybe we should use it in mongooseim ;)

pawlooss1 · 2023-02-27T10:56:08Z

src/cets_long.erl

+    Pid = spawn_mon(Info, Parent, Start),
+    try
+            case Catch of
+                true -> just_run_safely(Info#{what => long_task_failed}, Fun);


Putting what => long_task_failed in this line suggests that our task has already failed. I think it would be more straightforward if we just put it directly in the logging in line 71.

src/cets_call.erl

pawlooss1 · 2023-02-27T12:48:16Z

rebar.config

+     no_opaque, no_fail_call, no_contracts, no_behaviours,
+     no_undefined_callbacks, unmatched_returns, error_handling,
+     underspecs
+     % overspecs, specdiffs


Please either remove or uncomment this line.

pawlooss1 · 2023-02-27T12:50:01Z

src/cets.app.src

+  {registered, []},
+  {applications, [kernel, stdlib]},
+  {env, []}
+% {mod, {cets_app, []}


Please either remove or uncomment this line.

src/cets_discovery_file.erl

pawlooss1 · 2023-02-28T12:52:28Z

src/cets_join.erl

+    Info2 = Info#{local_pid => LocalPid,
+                  remote_pid => RemotePid, remote_node => node(RemotePid)},
+    F = fun() -> join1(LockKey, Info2, LocalPid, RemotePid) end,
+    cets_long:run_safely(Info2#{what => join_failed}, F).


If I get this correctly, what key of this map is going to be overwritten in cets_long.erl in every logging line. Could you check it?

If I get this correctly, what key of this map is going to be overwritten in cets_long.erl in every logging line. Could you check it?

What's more, this can be misleading especially when debugging/tracing. Finding what => join_failed in the arguments would suggest that something has already failed.

Additionally, there will be more cets_long calls inside this cets_long call. I don't think we need doubled monitors. Maybe skip the outer cets_long entirely and just catch errors?

pawlooss1 · 2023-02-28T13:11:54Z

test/cets_SUITE.erl

+end_per_testcase(_, _Config) ->
+    ok.
+
+test_multinode(Config) ->


This test checks a few thing, e.g. joining nodes (both before and after adding data), inserting values, deleting values, bulk operations. Could you split it to minimal test cases where each one tests exactly one thing?

I added more tests for each operation

Ok, but it just doesn't help with the readability of this particular one.

chrzaszcz

It looks promising 🙂 I added some comments.

chrzaszcz · 2022-09-26T12:39:20Z

README.md

+- `delete(Server, Key)` - deletes an object from the table.
+- `delete_many(Server, Keys)` - deletes several objects from the table.
+
+cets_join module contains the merging logic.


Suggested change

cets_join module contains the merging logic.

`cets_join` module contains the merging logic.

chrzaszcz · 2022-09-26T12:39:31Z

README.md

+
+cets_join module contains the merging logic.
+
+cets_discovery module handles search of new nodes.


Suggested change

cets_discovery module handles search of new nodes.

`cets_discovery` module handles search of new nodes.

...and it would be good to format the remainder of this page as well.

chrzaszcz · 2023-02-28T12:58:25Z

src/cets.erl

+stop(Server) ->
+    gen_server:stop(Server).
+
+-spec dump(server_ref()) -> Records :: [tuple()].


Suggested change

-spec dump(server_ref()) -> Records :: [tuple()].

-spec dump(table_name()) -> Records :: [tuple()].

run_test.sh

src/cets.erl

chrzaszcz · 2023-03-01T12:39:44Z

.gitignore

@@ -0,0 +1,4 @@
+_build/
+rebar3


There is no rebar3 in the repo.

chrzaszcz · 2023-03-01T17:46:42Z

src/cets.erl

+replicate(From, Msg, #{mon_tab := MonTab, other_servers := Servers}) ->
+    %% Reply would be routed directly to FromPid
+    Msg2 = {remote_op, From, Msg},
+    replicate2(Servers, Msg2),


I don't think we gain anything by the increased amount of code and the additional function with a 2 suffix. lists:foreach is also fine for me - easier to read, debug, understand...

Suggested change

replicate2(Servers, Msg2),

[send_to_remote(RemotePid, Msg2) || RemotePid <- Servers],

chrzaszcz · 2023-03-01T17:48:29Z

src/cets.erl

+    ok.
+
+apply_backlog(State = #{backlog := Backlog}) ->
+    apply_backlog_ops(lists:reverse(Backlog), State),


Same as above.

src/cets_discovery.erl

chrzaszcz · 2023-03-02T07:54:23Z

src/cets_discovery.erl

+    State#{timer_ref := TimerRef}.
+
+cancel_old_timer(#{timer_ref := OldRef}) when is_reference(OldRef) ->
+    _ = erlang:cancel_timer(OldRef),


What is the point of this pattern match? It has no effect.

prevents from dialyzer warning.

Which option triggers this warning? I see a lot of them enabled. Sometimes it might be better to disable a check if it forces us to write unnecessary code.

chrzaszcz · 2023-03-02T10:28:59Z

src/cets.erl

+init({Tab, Opts}) ->
+    process_flag(message_queue_data, off_heap),
+    MonTab = list_to_atom(atom_to_list(Tab) ++ "_mon"),
+    _ = ets:new(Tab, [ordered_set, named_table, public]),


Another note: why ordered_set? It slows down read time. Also: how about allowing some customisation instead of hardcoding the table type?

ordered_set is required for ejabberd_sm to work - for reads all sessions of the user, for example.

Address some review comments

Test each operation of test_multinode separately

prevent cets from killing the processes (and losing ETS data) in the event of accidentally calling it with LocalPid same as RemotePid

Expose mon_pid in cets:info

chrzaszcz

Looks good 👍

chrzaszcz · 2023-03-23T07:47:54Z

src/cets_join.erl

+        {ok, LocalDump} = remote_or_local_dump(LocalPid),
+        {ok, RemoteDump} = remote_or_local_dump(RemotePid),


Actually in some tests the local one is the remote one, they are swapped, that's what I meant.

chrzaszcz · 2023-03-23T07:58:51Z

test/cets_SUITE.erl

+end_per_testcase(_, _Config) ->
+    ok.
+
+test_multinode(Config) ->


Ok, but it just doesn't help with the readability of this particular one.

chrzaszcz · 2023-03-23T08:00:17Z

test/cets_SUITE.erl

+    {ok, Pid2} = start(Node2, Tab),
+    {ok, Pid3} = start(Node3, Tab),
+    {ok, Pid4} = start(Node4, Tab),
+    ok = join(Node1, Tab, Pid3, Pid1),


This happens a few times: the first pid is remote, and the second is local, while in the join function it is the other way around. Maybe don't swap the order to make it easier to understand?

arcusfelis added 6 commits May 9, 2022 13:02

Add an empty application and a test suite

04d46de

Add ETS replication logic (working simple test)

eb2ee50

Use persistent term for metadata Monitor a proxy process

Add logging and long task tracking

b5eedc8

Lock joining

5951b7d

Report time to get the joining lock

Add auto-discovery support

e3011b4

Add handle_down

36c75e4

arcusfelis force-pushed the rebased branch from cd0116d to ebe123a Compare May 9, 2022 11:08

arcusfelis mentioned this pull request May 9, 2022

Alternative backend to Mnesia - CETS esl/MongooseIM#3629

Merged

NelsonVides self-requested a review May 9, 2022 11:26

arcusfelis force-pushed the rebased branch 3 times, most recently from db4ba52 to 631af65 Compare May 9, 2022 17:34

arcusfelis added 18 commits May 9, 2022 19:40

Add delete and multidelete

89d4caa

Allow to add tables to disco dynamically

bcc77c3

Register name for disco

Use nosuspend when sending messages

00b5f2f

Use run_safely

172bd79

Remove try-catch

Support discovery backends

272a81d

Use OTP logger

3472f13

Remove remote_just_add_node_to_schema function

d97032e

Add servers_to_pids and has_remote_pid helper functions

60ed57d

Split handle_join

3dbaf3b

Route deletes/inserts though the local server

03b1f96

Remove kiss_pt

813d106

Properly pause to avoid race conditions

519dfaa

Move joining logic into a separate file

90dd5e0

Dedup code with short_call

0f668b8

Merge insert_to_remote_nodes and delete_from_remote_nodes

8dafa78

Reorder clauses in handle_call

af27a2f

Fix stop output in kiss_proxy

8615e89

Reorder functions for better readability

bf1c1e0

arcusfelis added 7 commits May 9, 2022 19:40

Avoid deadlocks in sync/1

49dbdde

Add Github CI

9219a4f

Update readme

Add insert_many_request and insert_many API functions

91a0c92

Allow multiple pauses

91ff0b9

Add dialyzer task for CI

8a0799e

Separate operations for insert and insert_many

e7a25c1

Separate operations for delete and delete_many Add more types

Add unpause_twice and pause_multiple_times testcases

e203243

arcusfelis force-pushed the rebased branch from 631af65 to e203243 Compare May 9, 2022 17:40

arcusfelis requested a review from chrzaszcz May 9, 2022 17:42

arcusfelis added 4 commits May 9, 2022 19:54

Use long_call function

36fc17f

Move call specific code into cets_call

90e1e65

To split the business logic from the helpers logic

Update file docs

3cb135f

Fix dialyzer

68a6547

pawlooss1 reviewed Feb 28, 2023

View reviewed changes

chrzaszcz reviewed Mar 2, 2023

View reviewed changes

arcusfelis added 10 commits March 6, 2023 20:45

Autoformat with erlfmt

65cf647

Update erlang version to 25

2e5c835

Add specs for cets_discovery

be8c904

Address some review comments

Add more granular tests

dab33e5

Test each operation of test_multinode separately

Remove unnecessary cets_long:run call in join

7dcb8d0

Add comment about matching result for dialyzer

4d88357

Add specs for cets_long / cets_join

2136839

Handle join_with_the_same_pid

9efd5af

prevent cets from killing the processes (and losing ETS data) in the event of accidentally calling it with LocalPid same as RemotePid

Stop monitor process properly

665ed35

Expose mon_pid in cets:info

Remove unused cets_long:run/2

b9015bd

chrzaszcz approved these changes Mar 23, 2023

View reviewed changes

Fix the order of arguments in join in tests

3d1e1d6

chrzaszcz merged commit d90265d into main Mar 23, 2023

NelsonVides deleted the rebased branch July 26, 2023 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial code #1

Add initial code #1

arcusfelis commented May 9, 2022 •

edited

Loading

pawlooss1 left a comment

pawlooss1 Feb 27, 2023

arcusfelis Mar 6, 2023

pawlooss1 Feb 27, 2023

pawlooss1 Feb 27, 2023

pawlooss1 Feb 27, 2023

pawlooss1 Feb 28, 2023

chrzaszcz Feb 28, 2023

chrzaszcz Mar 1, 2023

pawlooss1 Feb 28, 2023

arcusfelis Mar 20, 2023

chrzaszcz Mar 23, 2023

chrzaszcz left a comment

chrzaszcz Sep 26, 2022

chrzaszcz Sep 26, 2022

chrzaszcz Feb 28, 2023

chrzaszcz Mar 1, 2023

chrzaszcz Mar 1, 2023

chrzaszcz Mar 1, 2023

chrzaszcz Mar 2, 2023

arcusfelis Mar 6, 2023

chrzaszcz Mar 23, 2023

chrzaszcz Mar 2, 2023

arcusfelis Mar 6, 2023

chrzaszcz left a comment

chrzaszcz Mar 23, 2023

chrzaszcz Mar 23, 2023

chrzaszcz Mar 23, 2023

	cets_join module contains the merging logic.
	`cets_join` module contains the merging logic.


		cets_join module contains the merging logic.

		cets_discovery module handles search of new nodes.

	cets_discovery module handles search of new nodes.
	`cets_discovery` module handles search of new nodes.

	-spec dump(server_ref()) -> Records :: [tuple()].
	-spec dump(table_name()) -> Records :: [tuple()].

	replicate2(Servers, Msg2),
	[send_to_remote(RemotePid, Msg2) \|\| RemotePid <- Servers],

		{ok, LocalDump} = remote_or_local_dump(LocalPid),
		{ok, RemoteDump} = remote_or_local_dump(RemotePid),

		@@ -0,0 +1,4 @@
		_build/
		rebar3

Add initial code #1

Add initial code #1

Conversation

arcusfelis commented May 9, 2022 • edited Loading

pawlooss1 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

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

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

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

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

arcusfelis commented May 9, 2022 •

edited

Loading