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

Improve bridges design #1849

Merged
merged 6 commits into from Sep 22, 2018

Conversation

@turtleDeng
Copy link
Collaborator

turtleDeng commented Sep 21, 2018

No description provided.

@huangdan huangdan requested review from emqplus, terry-xiaoyu and spring2maz Sep 21, 2018
@coveralls

This comment has been minimized.

Copy link

coveralls commented Sep 21, 2018

Pull Request Test Coverage Report for Build 3324

  • 3 of 58 (5.17%) changed or added relevant lines in 1 file are covered.
  • 124 unchanged lines in 7 files lost coverage.
  • Overall coverage increased (+0.1%) to 55.176%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/emqx_bridge.erl 3 58 5.17%
Files with Coverage Reduction New Missed Lines %
src/emqx_ws_connection.erl 2 36.11%
src/emqx_misc.erl 2 93.33%
src/emqx_gc.erl 2 91.67%
src/emqx_bridge.erl 3 12.17%
src/emqx_keepalive.erl 6 62.5%
src/emqx_connection.erl 40 56.34%
src/emqx_session.erl 69 68.46%
Totals Coverage Status
Change from base Build 3269: 0.1%
Covered Lines: 2292
Relevant Lines: 4154

💛 - Coveralls
@emqplus emqplus self-assigned this Sep 21, 2018
@emqplus emqplus added the Enhancement label Sep 21, 2018
@emqplus emqplus added this to the 3.0-beta.3 milestone Sep 21, 2018
{reply, Forwards, State};

handle_call({add_forward, Topic}, _From, State = #state{forwards = Forwards}) ->
case emqx_topic:validate({filter, Topic}) andalso (not lists:member(Topic, Forwards)) of

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 21, 2018

Contributor

emqx_topic:validate/1 is side-effect free except for error exceptions.
the exceptions may cause gen_server to crash, move validation to the gen_server:call wrapper ?

This comment has been minimized.

Copy link
@emqplus

emqplus Sep 21, 2018

Contributor

Agree:)

{reply, Subscriptions, State};

handle_call({add_subscription, Topic, Qos}, _From, State = #state{subscriptions = Subscriptions, client_pid = ClientPid}) ->
case emqx_topic:validate({filter, Topic}) andalso (not lists:keymember(Topic, 1, Subscriptions)) of

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 21, 2018

Contributor

same here.

@@ -787,7 +787,7 @@ connected(cast, ?SUBACK_PACKET(PacketId, Properties, ReasonCodes),
connected(cast, ?UNSUBACK_PACKET(PacketId, Properties, ReasonCodes),
State = #state{subscriptions = Subscriptions}) ->
case take_call({unsubscribe, PacketId}, State) of
{value, #call{from = From, req = {_, Topics}}, NewState} ->
{value, #call{from = From, req = {_, _, Topics}}, NewState} ->

This comment has been minimized.

Copy link
@spring2maz

spring2maz Sep 21, 2018

Contributor

this would be easier to read/troubleshoot if a macro is used here.
e.g. ?UNSUB_REQ(_, _, Topics)

## Bridge address: node name for local bridge, host:port for remote.
##
## Value: String
## Example: emqx@127.0.0.1, 127.0.0.1:1883
bridge.edge.address = 127.0.0.1:1883
bridge.bridge1.address = 127.0.0.1:1883

This comment has been minimized.

Copy link
@emqplus

emqplus Sep 22, 2018

Contributor

bridge.aws.address = ...

## Bridge address: node name for local bridge, host:port for remote.
##
## Value: String
## Example: emqx@127.0.0.1, 127.0.0.1:1883
bridge.edge.address = 127.0.0.1:1883
bridge.bridge1.address = 127.0.0.1:1883

## Protocol version of the bridge.
##
## Value: Enum
## - mqtt5
## - mqtt4
## - mqtt3

This comment has been minimized.

Copy link
@emqplus

emqplus Sep 22, 2018

Contributor
## - mqttv5
## - mqttv4
## - mqttv3
##
## Value: String
## bridge.edge.ciphers = ECDHE-ECDSA-AES256-GCM-SHA384,ECDHE-RSA-AES256-GCM-SHA384
bridge.bridge1.keepalive = 10s

This comment has been minimized.

Copy link
@emqplus

emqplus Sep 22, 2018

Contributor

60s

## Value: Duration
## Default: 10 seconds
bridge.cloud.keepalive = 10s
bridge.bridge1.subscription.1.topic = $share/cmd/topic1

This comment has been minimized.

Copy link
@emqplus

emqplus Sep 22, 2018

Contributor

Remove the '$share/' prefix

show_subscriptions(Name) ->
gen_server:call(name(Name), show_subscriptions).

add_subscription(Name, Topic, Qos) ->

This comment has been minimized.

Copy link
@emqplus

emqplus Sep 22, 2018

Contributor

-spec(add_subscription(Name, Topic, QoS) -> ok | {error, Reason}).

emqx_broker:subscribe(Topic),
{reply, ok, State#state{forwards = [Topic | Forwards]}};
false ->
{reply, fail, State}

This comment has been minimized.

Copy link
@emqplus

emqplus Sep 22, 2018

Contributor

{error, already_exists}

emqx_broker:unsubscribe(Topic),
{reply, ok, State#state{forwards = lists:delete(Topic, Forwards)}};
false ->
{reply, fail, State}

This comment has been minimized.

Copy link
@emqplus

emqplus Sep 22, 2018

Contributor

{reply, ok, State}

turtleDeng added 2 commits Sep 22, 2018
@terry-xiaoyu terry-xiaoyu merged commit 8f35d13 into emqx30 Sep 22, 2018
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 55.176%
Details
@turtleDeng turtleDeng deleted the emqx30-deng branch Oct 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.