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

NATS transporter and compatibility with moleculer-js #56

Closed
wants to merge 24 commits into from

Conversation

dehypnosis
Copy link

@dehypnosis dehypnosis commented May 3, 2019

1. Related issues

is NATS supported currently? #53
Moleculer JS compatibility #55

2. Includes

2-1. NATS transporter implementation

"nats://uri" transporter configuration shall be respected

2-2. moleculer-js compatibility and network stability

  • Fix event payload field to "data" from "params"

  • Fix "services" field of INFO packet

    • modify actions field to map[string]interface{} from []interface{}
    • modify action "name" field from "action.name" to "my.service.action.name"
    • add "rawName" field to action which would like "action.name"
  • INFO packet shall not embed "seq", "cpuSeq" fields
    by https://github.com/moleculerjs/moleculer/blob/master/docs/PROTOCOL.md#info

  • INFO packet should return internal services as well ($node)
    I'm curious about the reason you hided internal services from packet.
    There might be the reason I do not know, but actually this commit is needed for our teams internal application, which collects all the node of cluster and gathers health status.

  • implement $node.health and $node.options very incompletely

  • make remoteNodeInfoReceived() to be able to parse "actions", "events" map[string]interfaces{} fields from INFO packet

  • temporarily fix checkOfflineNodes() timeout range for network stability among moleculer-js.
    moleculer-js set 10min as default configuration for this.
    It would be better other client option can be provided.

  • Adjust DefaultConfig.HeartbeatFrequency/HeartbeatTimeout
    Set these default options as 5s/15s for network stability among moleculer-go and moleculer-js nodes.
    Without this, moleculer-go nodes continue to be connected and disconnected from moleculer-js nodes.

2-3. Miscellaneous

  • Update discoverNodeID() to follow convention of moleculer-js as like "{hostname}-{integer:5}"
  • Prevent localNodeID being changed upon runtime. (bug)

3. Compatibility status

I made a moleculer-go service with forked version of mine with NATS transporter.

works

  • discovery works well and networking is stable for recent 48 hours
  • event subscription, emit, broadcast works well
  • action req/res works well

needs

  • event subscription with pattern matching like "my.event.**"
  • access context.meta from action handler
  • compatible error format

Screen Shot 2019-05-03 at 11 53 13 AM

IT IS AMZAING!

4. Suggestion

I know you are currently working on ActionSchema thing which can validate action params. This feature is amazing and our team also using this feature for every actions.

Actually I made a API gateway (which is different from moleculer-web) and the console application which has a feature making API documents from service schema and action schema like as below.
Surely this feature needs a normalized action params schema.

Screen Shot 2019-05-03 at 11 31 42 AM

if action.params schema can follow icebob/fastest-validator which is included in moleculer-js, It would be great. Because I think params itself can be a kind of protocol for moleculer service schema.

5. Sorry about

I couldn't make any test cases for above commits and codes for compatibility lack deep comprehension about moleculer-go because i am currently so busy rapidly implementing our companies' services with forked version of moleculer-go.

So this PR may cannot be accepted, and it would be no problem. I just hope this PR can be even a hint for moleculer-js compatibility.

And really appreciate for your hard work!!
Thanks!

slaterx and others added 24 commits February 3, 2019 20:47
…ariables

Update discoverNodeID() to follow convention of moleculer-js as like "{hostname}-{integer:5}"
also prevent localNodeID being changed upon runtime.

Adjust DefaultConfig.HeartbeatFrequency/HeartbeatTimeout as equal to
one of moleculer-js to network stability among moleculer-go and moleculer-js nodes.
without this, moleculer-go nodes continue to be connected and disconnected.
- HeartBeatFrequency: 5s
- HeartbeatTimeout: 15s
…leculer-js

modify actions field to map[string]interface{} from []interface{}
modify action "name" field from "action.name" to "my.service.action.name"
add "rawName" field to action which would like "action.name"
… map[string]interfaces{} fields from INFO packet

And temporarily fix checkOfflineNodes() timeout range to network
stability, would be better other client option can be provided.
Copy link
Member

@slaterx slaterx left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@slaterx
Copy link
Member

slaterx commented May 3, 2019

I've began to develop a fix for #54, but I will wait for your merge to be completed, so I can incorporate your NATS transporter into the work.

@pentateu
Copy link
Member

pentateu commented May 4, 2019

@dehypnosis thank you for the contribution.

I'll have a look at the code and try to retrofit the test cases. IF that is too much work, I'll use your code as a starting point to fix the compatibility issues and other issues you have pointed out.

Regardless of the approach, you will have a moleculer-go version soon that is 100% compatible and stable with moleculer JS.

We still need to write our contribution guidelines, but for future PR please try to do one feature/fix per each PR. That makes us able to approve/fix PRs quickly and move faster :)

@pentateu pentateu mentioned this pull request May 4, 2019
@pentateu
Copy link
Member

pentateu commented May 4, 2019

@dehypnosis NATS transporter looks great. I'll merge that as part of PR #57 and will create a new one for Moleculer JS compatibility.

@pentateu
Copy link
Member

pentateu commented May 7, 2019

@dehypnosis all issues you have highlighted here, and requests were implemented on PR #59

Exceptions:

These 2 requires a bit of design consideration, but they will come out soon.

@pentateu pentateu closed this May 7, 2019
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

3 participants