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

Add socketOptions to RMQ connection for client and server #1549

Merged
merged 13 commits into from Apr 10, 2019

Conversation

@Upperfoot
Copy link
Contributor

commented Feb 12, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

You cannot specify the socket (connection) options detailed in https://www.squaremobius.net/amqp.node/channel_api.html#connect

Issue Number: N/A

What is the new behavior?

You will now be able to specify the socket options (i.e. adding noDelay)

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I came across this not being possible when I was looking at tuning our system, so decided to add it!

Upperfoot added 6 commits Feb 12, 2019
@Upperfoot Upperfoot referenced this pull request Feb 12, 2019
1 of 1 task complete
Upperfoot added 3 commits Feb 12, 2019
@coveralls

This comment has been minimized.

Copy link

commented Feb 12, 2019

Pull Request Test Coverage Report for Build 2017

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • 117 unchanged lines in 29 files lost coverage.
  • Overall coverage decreased (-0.2%) to 93.648%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/client/client-rmq.ts 0 1 0.0%
Files with Coverage Reduction New Missed Lines %
packages/common/utils/merge-with-values.util.ts 1 85.71%
packages/common/decorators/modules/single-scope.decorator.ts 1 87.5%
packages/common/decorators/http/route-params.decorator.ts 1 87.8%
packages/microservices/client/client-proxy.ts 1 94.87%
packages/common/pipes/validation.pipe.ts 1 93.75%
packages/core/interceptors/interceptors-context-creator.ts 1 97.44%
packages/microservices/server/server.ts 1 97.44%
packages/websockets/socket-server-provider.ts 1 92.86%
packages/microservices/server/server-mqtt.ts 1 95.83%
packages/core/router/router-execution-context.ts 1 95.24%
Totals Coverage Status
Change from base Build 1511: -0.2%
Covered Lines: 2996
Relevant Lines: 3143

💛 - Coveralls
@@ -49,6 +51,9 @@ export class ClientRMQ extends ClientProxy {
this.queueOptions =
this.getOptionsProp<RmqOptions>(this.options, 'queueOptions') ||
RQM_DEFAULT_QUEUE_OPTIONS;
this.socketOptions =
this.getOptionsProp<RmqOptions>(this.options, 'socketOptions') ||
RQM_DEFAULT_SOCKET_OPTIONS;

This comment has been minimized.

Copy link
@kamilmysliwiec

kamilmysliwiec Feb 18, 2019

Member

do we have to use an additional class property?

This comment has been minimized.

Copy link
@Upperfoot

Upperfoot Feb 18, 2019

Author Contributor

Thanks @kamilmysliwiec, your suggestion on it being called upon during the createClient is a much cleaner method, less properties flying around. I kind of wanted to keep parity with queueOptions but after looking at it further socketOptions wouldn't really be used outside of the context of createClient, good shout! 👍

@@ -93,7 +98,7 @@ export class ClientRMQ extends ClientProxy {
}

public createClient<T = any>(): T {
return rqmPackage.connect(this.urls) as T;
return rqmPackage.connect(this.urls, this.socketOptions) as T;

This comment has been minimized.

Copy link
@kamilmysliwiec

kamilmysliwiec Feb 18, 2019

Member

maybe we could make socketOptions a local variable and just pluck this property inside the createClient method:

const socketOptions = this.getOptionsProp<RmqOptions>(this.options, 'socketOptions');
@@ -25,6 +25,7 @@ export const RQM_DEFAULT_QUEUE = 'default';
export const RQM_DEFAULT_PREFETCH_COUNT = 0;
export const RQM_DEFAULT_IS_GLOBAL_PREFETCH_COUNT = false;
export const RQM_DEFAULT_QUEUE_OPTIONS = {};
export const RQM_DEFAULT_SOCKET_OPTIONS = {};

This comment has been minimized.

Copy link
@kamilmysliwiec

kamilmysliwiec Feb 18, 2019

Member

could be potentially removed? (just use undefined when not explicitly defined)

@@ -43,6 +45,9 @@ export class ServerRMQ extends Server implements CustomTransportStrategy {
this.queueOptions =
this.getOptionsProp<RmqOptions>(this.options, 'queueOptions') ||
RQM_DEFAULT_QUEUE_OPTIONS;
this.socketOptions =
this.getOptionsProp<RmqOptions>(this.options, 'socketOptions') ||
RQM_DEFAULT_SOCKET_OPTIONS;

This comment has been minimized.

Copy link
@kamilmysliwiec

kamilmysliwiec Feb 18, 2019

Member

everything same as in the client ^ class

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Feb 18, 2019

hey @Upperfoot, thanks for the contribution! I left a few comments.

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Ping 🏓

Upperfoot added 4 commits Mar 20, 2019
@Upperfoot

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Pong! 🏓 @kamilmysliwiec

@kamilmysliwiec

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Thanks @Upperfoot :)

@kamilmysliwiec kamilmysliwiec merged commit b0088db into nestjs:master Apr 10, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.2%) to 93.648%
Details
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.