Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement mria:sync_transaction/4,3,2 #136

Merged

Conversation

SergeTupchiy
Copy link
Contributor

When called on a replicant node, this function blocks a caller process until the transaction is replicated on that node or a timeout occurs. Otherwise, it behaves the same way as mria:transaction/3,2

Closes: EMQX-9102

@SergeTupchiy SergeTupchiy self-assigned this Apr 6, 2023
@SergeTupchiy SergeTupchiy force-pushed the EMQX-9102-sync-transaction-replicant branch 6 times, most recently from 9070b76 to e03e83e Compare April 11, 2023 19:38
src/mria.erl Outdated Show resolved Hide resolved
@SergeTupchiy SergeTupchiy force-pushed the EMQX-9102-sync-transaction-replicant branch 2 times, most recently from a07b662 to 739bcb8 Compare April 12, 2023 11:40
@SergeTupchiy SergeTupchiy marked this pull request as ready for review April 12, 2023 11:42
@SergeTupchiy SergeTupchiy force-pushed the EMQX-9102-sync-transaction-replicant branch from 739bcb8 to 88ce38d Compare April 12, 2023 13:05
@SergeTupchiy SergeTupchiy marked this pull request as draft April 12, 2023 16:13
When called on a replicant node, this function blocks a caller process until
the transaction is replicated on that node or a timeout occurs.
Otherwise, it behaves the same way as mria:transaction/3,2

Closes: EMQX-9102
@SergeTupchiy SergeTupchiy force-pushed the EMQX-9102-sync-transaction-replicant branch from 88ce38d to 02c269b Compare April 12, 2023 20:19
@SergeTupchiy SergeTupchiy marked this pull request as ready for review April 12, 2023 20:23
@SergeTupchiy SergeTupchiy removed their assignment Apr 12, 2023
@@ -600,3 +639,69 @@ db_nodes_maybe_rpc() ->
[]
end
end.

sync_replicant_trans(Shard, Function, Args, ReplTimeout) ->
case mria_rlog:wait_for_shards([Shard], ReplTimeout) of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should actually measure time spent during this stage, as well as other stages, and do something

TimeLeft = ReplTimeout - TWaitingForShards,
...
maybe_retry_waiting(..., TimeLeft)

in order to not exceed the user-specified timeout value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also thought about it, will definitely add it.

Copy link
Contributor Author

@SergeTupchiy SergeTupchiy Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.
Please note that I deliberately haven't measured rpc_to_core_node time, since it uses retries / cool down based on mria_config values.
If we want to limit total function execution to a user-specified timeout, I'll also update rpc_to_core_node implementation.

src/mria.erl Show resolved Hide resolved
src/mria_rlog.hrl Outdated Show resolved Hide resolved
mria_rlog:wait_for_shards([Shard], infinity),
mnesia:transaction(fun() ->
Res = apply(Fun, Args),
ok = mnesia:write(ReplyTo),
Copy link
Member

@ieQu1 ieQu1 Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's possible to inject this write on the replicant side? It will lead to better code reuse, since it will use the existing transactional_wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks for a good suggestion, fixed:

 SyncFun = fun(Args_) ->                                                
                  Res_ = apply(Fun, Args_),                            
                  ok = mnesia:write(ReplyTo),                          
                  Res_                                                 
          end,                                                         
CoreRes = rpc_to_core_node(Shard, mria_upstream, transactional_wrapper,
                           [Shard, SyncFun, [Args]]),

@SergeTupchiy SergeTupchiy force-pushed the EMQX-9102-sync-transaction-replicant branch from 65f37f7 to 53f9a2c Compare April 13, 2023 19:05
src/mria.erl Outdated
@@ -368,6 +371,42 @@ ro_transaction(Shard, Fun) ->
end
end.

%% @doc Synchronous transaction.
%% This function has a behavior different from transaction/2,3 only when called on a replicant node.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
%% This function has a behavior different from transaction/2,3 only when called on a replicant node.
%% This function has a behavior different from `transaction/2,3' only when called on a replicant node.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/mria.erl Outdated
@@ -368,6 +371,42 @@ ro_transaction(Shard, Fun) ->
end
end.

%% @doc Synchronous transaction.
%% This function has a behavior different from transaction/2,3 only when called on a replicant node.
%% When a process on a replicant node calls sync_transaction/4,3,2, it will be blocked
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
%% When a process on a replicant node calls sync_transaction/4,3,2, it will be blocked
%% When a process on a replicant node calls `sync_transaction/4,3,2', it will be blocked

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/mria.erl Outdated
%% When a process on a replicant node calls sync_transaction/4,3,2, it will be blocked
%% until the transaction is replicated on that node or a timeout occurs.
%% It should be noted that a ReplTimeout doesn't control the total maximum execution time
%% of sync_transaction/4,3,2. It is only used to wait for a transaction to be imported
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
%% of sync_transaction/4,3,2. It is only used to wait for a transaction to be imported
%% of `sync_transaction/4,3,2'. It is only used to wait for a transaction to be imported

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

src/mria.erl Outdated
%% to the local node which originated the transaction.
%% Thus, the total execution time may be significantly higher,
%% e. g. when rpc to core node was slow and/or retried.
%% Moreover, when {timeout, t_result(A)} is returned, the result can be successful.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
%% Moreover, when {timeout, t_result(A)} is returned, the result can be successful.
%% Moreover, when `{timeout, t_result(A)}' is returned, the result can be successful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@SergeTupchiy SergeTupchiy force-pushed the EMQX-9102-sync-transaction-replicant branch 2 times, most recently from bca4d30 to ab50fe6 Compare April 14, 2023 18:17
src/mria.erl Show resolved Hide resolved
src/mria_schema.erl Outdated Show resolved Hide resolved
@SergeTupchiy SergeTupchiy force-pushed the EMQX-9102-sync-transaction-replicant branch from ab50fe6 to 2e7fe50 Compare April 18, 2023 07:56
@SergeTupchiy SergeTupchiy force-pushed the EMQX-9102-sync-transaction-replicant branch from 2e7fe50 to 9dcfae2 Compare April 18, 2023 20:02
@@ -368,6 +371,42 @@ ro_transaction(Shard, Fun) ->
end
end.

%% @doc Synchronous transaction.
%% This function has a behavior different from 'transaction/2,3' only when called on a replicant node.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small remark for the future: edoc uses LaTeX-style quoting for code: `' rather than ''.
Let's ignore it, since we don't build edoc for mria

@SergeTupchiy SergeTupchiy merged commit 01b66cc into emqx:main Apr 24, 2023
@ieQu1 ieQu1 mentioned this pull request Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants