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: cluster linking WIP #12879

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SergeTupchiy
Copy link
Contributor

Release version: v/e5.7.0

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

@SergeTupchiy
Copy link
Contributor Author

NOTE: still not even a draft quality, but it's constantly updated..

@SergeTupchiy SergeTupchiy force-pushed the EMQX-11967-cross-cluster-route-replication branch from 003a017 to 2b77999 Compare April 18, 2024 16:00
@SergeTupchiy SergeTupchiy changed the title feat: cluster link prototype WIP feat: cluster linking WIP Apr 19, 2024
@SergeTupchiy SergeTupchiy force-pushed the EMQX-11967-cross-cluster-route-replication branch from 2b77999 to 23b44f7 Compare April 19, 2024 18:27
%% we don't expect the message to be forwarded from an older EMQX release,
%% that doesn't set extra = #{} by default.
with_sender_name(#message{extra = Extra} = Msg, ClusterName) when is_map(Extra) ->
Msg#message{extra = Extra#{sender_link => ClusterName}}.
Copy link
Member

@zmstone zmstone Apr 22, 2024

Choose a reason for hiding this comment

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

Suggested change
Msg#message{extra = Extra#{sender_link => ClusterName}}.
Msg#message{extra = Extra#{link_origin => ClusterName}}.

@@ -246,7 +246,7 @@ publish(Msg) when is_record(Msg, message) ->
[];
Msg1 = #message{topic = Topic} ->
PersistRes = persist_publish(Msg1),
route(aggre(emqx_router:match_routes(Topic)), delivery(Msg1), PersistRes)
route(aggre(emqx_router:match_routes(Topic), Msg1), delivery(Msg1), PersistRes)
Copy link
Member

Choose a reason for hiding this comment

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

maybe return two lists, one for regular routes, another for external routes.
then drop external routes if Msg1 is originated from a link delivery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

_ =
case emqx_cluster_link_mqtt:decode_forwarded_msg(Payload) of
#message{} = ForwardedMsg ->
emqx_broker:publish(with_sender_name(ForwardedMsg, ClusterName));
Copy link
Member

Choose a reason for hiding this comment

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

should return {stop, DecodedMessage} from here ? otherwise the original message will be received by other hooks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks. This also avoids extraemqx_broker:publish/1 call. 🙌

ok.

put_hook() ->
emqx_hooks:put('message.publish', {?MODULE, on_message_publish, []}, ?HP_LOWEST).
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to add this hook with priority HP_SYS_MSGS (higher than message validation), to stop other hooks from intercepting ?LINK messages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

%% Not implemented yet
ok
end,
ok;
Copy link
Member

Choose a reason for hiding this comment

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

return {stop, []} here to terminate the hook chain.
NOTE: [] does not seem to be acceptable by the current hook caller, so we'll need to add a new case clause to support [] and [Message | Batch].

(batching is not supported for now, but we could use this interface for future extension. For now, [] is to terminate publish, and #message{...} when forward is accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@SergeTupchiy SergeTupchiy force-pushed the EMQX-11967-cross-cluster-route-replication branch 5 times, most recently from 6bb70c1 to 8154ab4 Compare April 24, 2024 13:40
@SergeTupchiy SergeTupchiy force-pushed the EMQX-11967-cross-cluster-route-replication branch from 8154ab4 to df2c352 Compare April 25, 2024 16:34
@keynslug keynslug force-pushed the EMQX-11967-cross-cluster-route-replication branch from 1aa1271 to df2c352 Compare May 6, 2024 16:41
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.

None yet

2 participants