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

Allows app to disable hello logs for the remote services #50

Merged
merged 2 commits into from
Feb 17, 2020

Conversation

superdevofficial
Copy link

Reduce the hello log from Cote discovery.

For the remote services, Cote dispatch "hello" logs to the others microservies. These logs aren't always useful. This pr allow developers to disabled "hello" log from Cote discovery, in order to reduce the bandwidth usage.

You can add "{ "cote": { "stopDispatchRemoteServices": true } }" in your Feathers configuration to prevent "hello" logs.
( The variable name can be changed, we weren't very inspired! )

@claustres
Copy link
Member

Hi and thanks for this PR. However I wonder if this not already possible if you initialize the module with options in the following form:

{
  cote: { client: true, server: false }
}

Indeed we set some defaults options for cote and then merge with any provided at initializtion https://github.com/kalisio/feathers-distributed/blob/master/src/index.js#L130.

Could you give it a try so that we don't add unrequired code ?

Thanks again

@superdevofficial
Copy link
Author

Hi !

The modification of the cote configuration doesn't give the same result.

If you add the configuration cote: { client: true, server: false } the entire microservice is affected and it won't dispatch is own services (hello messages). But with our pull request, the microservice still dispatch is own service without dispatching remote services.

Let me know if it's not clear.

Thanks

@claustres
Copy link
Member

OK thanks for the explanation. However I would suggest to implement it a little differently. Indeed, at this line you are mutating the options object attached to the app, which could lead to unexpected results if it is used later in the module lifecycle. It would be better to assign the required options on the locally allocated options object IMHO.

Let me know if it's not clear.

@claustres claustres merged commit eae006a into kalisio:master Feb 17, 2020
@claustres
Copy link
Member

Dear @superdevofficial, I am afraid we had to perform a major internal architecture change in order to tackle different but probably related issues like #48 & #45. We previously had one requester for each remote service while we now have one requester for eac hremote app dispatching requests for all its remote services. This should not however be a breaking change from the external point of view.

I did not keep your changes as I am not completely sure if the result will be the same if we apply your PR to this new kind of requesters. Main changes are in this commit, could you have a look and provide a new PR ?

Sorry for the inconvenience.

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.

2 participants