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

Move request response out of emqx client #2293

Merged
merged 4 commits into from Mar 13, 2019

Conversation

@spring2maz
Copy link
Contributor

spring2maz commented Mar 11, 2019

The implementation before this PR is overly Intrusive for emqx_client module,
because request-response is a client-to-client protocol,
also, emqx_client isn't a end-user client (but just a bridging tool).

Request-response for broker: the only thing that requires support is the
'Response-Information' message property field in CONNACK,
the field is optional according to spec, (and can be disregarded by clients if I understand correctly).

I have put a comment in where 'Response-Information' should go
in case we want to add it back in the future.

@gilbertwong96

This comment has been minimized.

Copy link
Contributor

gilbertwong96 commented Mar 11, 2019

Neatly implemented 👍

spring2maz added 4 commits Mar 9, 2019
The Response-Infomation in CONNACK was misinterpreated, it should
not be a broker's global config, instead, it should be per-client
or even per-session prefix/suffix mechanism
@spring2maz spring2maz force-pushed the move-request-response-out-of-emqx_client branch from 14f3991 to 0872fda Mar 11, 2019
@spring2maz

This comment has been minimized.

Copy link
Contributor Author

spring2maz commented Mar 11, 2019

rebased to resolve conflict.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 11, 2019

Pull Request Test Coverage Report for Build 4957

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 29 unchanged lines in 7 files lost coverage.
  • Overall coverage decreased (-0.6%) to 70.157%

Files with Coverage Reduction New Missed Lines %
src/emqx_session_sup.erl 1 48.72%
src/emqx_broker.erl 1 64.56%
src/emqx_mqtt_props.erl 2 16.44%
src/emqx_shared_sub.erl 2 75.59%
src/emqx_session.erl 2 71.52%
src/emqx_connection.erl 7 63.03%
src/emqx_client.erl 14 49.62%
Totals Coverage Status
Change from base Build 4951: -0.6%
Covered Lines: 3484
Relevant Lines: 4966

💛 - Coveralls
@gilbertwong96 gilbertwong96 merged commit 48450d1 into develop Mar 13, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/travis-ci/push The Travis CI build failed
Details
coverage/coveralls Coverage decreased (-0.6%) to 70.157%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@terry-xiaoyu terry-xiaoyu deleted the move-request-response-out-of-emqx_client branch Mar 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.