[WIP] NATS Streaming Server / STAN support

[OtaK]

Still some work needed on this.

It pretty much is done, except ending up the support for verbose mode which is 3/4 done.

Needs integration tests against a STAN server.

[thedodd]

Anything I can help out with on this PR? I'm happy to write tests and such if that is what is needed.

[OtaK]

@thedodd Hey! I'll get back to it soon, but feel free to write tests, I'd get them merged against this branch.

If I can make a recommendation on how to get started about tests, I'd recommend following the integration tests the NATS team is running: https://github.com/nats-io/node-nats-streaming/tree/master/test

[thedodd]

... moved "todo" comment here to #21.

[thedodd]

@OtaK just saw your response. It wasn't showing up earlier for some reason ... maybe I just needed to refresh :). Cool, sounds great. I just forked so that I can knock out a few of the above items to unblock myself. I'll open up a PR targeting your branch here.

[thedodd]

@OtaK sorry for spamming you so hard on this PR, but while I was doing some code review in this PR, I noticed that the streaming subscription (specifically in the subscribe method) is setup in such a way that it will automatically Ack received messages before pushing them onto the subscription stream for the user to handle.

I cross referenced this with the go stan implementation, and that client is built to support “auto ack” or “manual ack”.

In order to avoid data loss, manual ack is needed any time where the subscription handler may fail. We should build a pattern for “manual ack” here as well.

I need the functionality, so I’ll look into implementing this on my PR.

[OtaK]

No problem! Yeah I know I skipped on this feature, I went down the opinionated route to make stuff easier to implement but seems your use case was not covered.
I do think though that by default, it should be in "auto-ack" mode.

Thanks!

[thedodd]

@OtaK my changes are pretty much done in the PR I opened. Easiest way to review it would be to select only my commits, as there are commits from develop as well (to resolve the conflicts).

Let me know if there are any changes you would like to see. I’m happy to iterate on the patterns used.

[o0Ignition0o]

Hey Anthony :) It looks really great, thanks for your efforts even though the lib isn't really documented so far ! :) Otak is away for the weekend, he'll probably have a bit of time on Monday or something, so don't worry if he doesn't reply until then :) Again thanks a lot for your contribution , I hope he'll review it soon :)

…

On Fri, Jan 18, 2019, 20:20 Anthony Dodd ***@***.*** wrote: @OtaK <https://github.com/OtaK> my changes are pretty much done in the PR I opened. Easiest way to review it would be to select only my commits, as there are commits from develop as well (to resolve the conflicts). Let me know if there are any changes you would like to see. I’m happy to iterate on the patterns used. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJBvTrSkMMBrgfjK1kPbT8voEAIj7ddNks5vEh53gaJpZM4ZTMNY> .

[thedodd]

@o0Ignition0o thanks for the heads up. No rush.

[christophermaier]

Good morning! I was wondering what the ETA on this work might be? I've been hunting around for a NATS Streaming library for Habitat, and the non-streaming NATS in this library is working pretty well in my POC.

Thanks in advance!

[thedodd]

Hey @christophermaier. If you need to immediately start using nitox streaming, you can update your Cargo.toml to depend on nitox from the repo in this PR: #21

• it is my fork and I have been using it successfully along with message ack'ing.
• it is based on this branch, but resolves the merge conflicts, and adds manual message ack'ing.

One word of warning, I would recommend that you take a look at issue #24. It is not specific to streaming, but streaming is effected by this issue. If the nitox client is the only task you will be running on your tokio runtime, then nothing to worry about. If you have other tasks which need to run, the best workaround is to just create a separate tokio runtime for nitox. That will ensure none of your other tasks get blocked.

Let me know if there is anything I can help out with or clarify.

[christophermaier]

@thedodd Thanks for the reply (as well as the pointer to #24); I'll give this a spin tomorrow! 👍

[OtaK]

Reporting @thedodd's comment on the todo here for information;

• resolves the current (as of 2019.01.16) merge conflict in feature/nats-server.
• fixes a bug where streaming client IDs were being generated with .s in the name. Server was rejecting the client connections & consumers were being blocked. I'm a now able to consume messages off of a stream.

todo

• finish up with manual ack pattern.
  Just a couple of observations for moving forward:
• streaming modules & types could used some docs. I'll let you handle this one, @OtaK.
• looks like the streaming_protocol module is private, so accessing the StartPosition variants is not possible. Maybe that is intentional, but it would def be nice to have access to the enum during development.
• the MsgProto object (also generated by prost/protobuf) is in the internal streaming_protocol.rs file and can't be accessed by stream handlers.
• also, I see that the derive builder crate is used throughout (in existing code as well). I would recommend using the typed_builder as everything can be verified at compile time. This is super convenient, especially when dealing with futures. We can open another issue for this later.

I've merged #21, and rebased the whole branch on develop to get rid of the ugly merge.

I guess integration tests are coming up next!

I'll study a move on typed_builder, it seems super cool and less panicky :o

[thedodd]

+1

It looks like no other commits were made to this PR, and I have been using the branch successfully for a while now with no issues. I neglected to write any tests for the streaming functionality ... that is the only thing that I think we could do. Perhaps we could just open an issue for it though. Add it to one of the milestones perhaps.

[OtaK]

I'd really like to have tests ready before merging it.
Even if the feature is opt-in, it would be super cool to be confident that it works 100%.

[OtaK]

I just added the support for the verbose mode!
It works by holding off future resolution until a +OK message comes down the line.

Next step will be writing an integration test suite for STAN / NATS Streaming Server.