Add NATS replica client #596

delaneyj · 2024-07-31T18:24:14Z

NATS is a distributed application framework. It has the ability to store objects similar to S3/Minio in a replicated persistence store. This replica client supports this feature for Litestream.

Note
The replica_client_test.go was geared towards only being tested against file. Have moved from fs/os errors to Litestream specific ones. This meant updating the file replica client to use these as well.

benbjohnson

I added a few points of feedback. Can you wire this up to cmd/litestream and add some instructions on how to test it?

file/replica_client.go

nats/replica_client.go

benbjohnson · 2024-08-13T20:32:41Z

nats/replica_client.go

@@ -0,0 +1,402 @@
+package nats


This doesn't seem to be imported or usable by the cmd/litestream binary. Am I missing something?

I had hooked it up (i thought) in cmd/litestream/replicate.go around line 125. Apparently this code wasn't commited

nats/replica_client.go

betamos · 2024-08-29T12:17:13Z

Big NATS fan here (oh, and Litestream too of course). Just stumbled across this PR accidentally and looked through it briefly. Some thoughts:

Why auto-create the bucket? IIUC litestream wants you to provision resources yourself (like the S3 client), which is trivial with the NATS CLI. If litestream is resposible, NATS has tons of config options that could pollute the litestream config in the future (currently I see replica count, which isn't necessary for litestream to know about).
Related: Retention is set on the NATS bucket. Without official support for out-of-bounds retention, I think it's much safer to let litestream manage retention in the same way it does elsewhere, using periodic checks.
Only JWT authentication? I think user/pass should absolutely be supported as a base-case, it's very common. (It can technically be encoded in the URL but since user/pass fields exist in litestream.yml, it violates the principle of least surprise).

Apologies for stepping in unannounced. Let me know if I can help out any other way.

delaneyj · 2024-08-29T14:14:47Z

@betamos thanks for taking the time

Why auto-create the bucket? IIUC litestream wants you to provision resources yourself (like the S3 client), which is trivial with the NATS CLI. If litestream is resposible, NATS has tons of config options that could pollute the litestream config in the future (currently I see replica count, which isn't necessary for litestream to know about).

I can see it both ways, if it's a blocker I'll remove it.

Related: Retention is set on the NATS bucket. Without official support for out-of-bounds retention, I think it's much safer to let litestream manage retention in the same way it does elsewhere, using periodic checks.

This is a good point. In general you want to set limits on buckets but this goes to your previous point.

Only JWT authentication? I think user/pass should absolutely be supported as a base-case, it's very common. (It can technically be encoded in the URL but since user/pass fields exist in litestream.yml, it violates the principle of least surprise).

I teach NATS usage at Synadia to customers. We highly recommend JWT only. User/pass, while supported currently support it really shouldn't be used. It's very easy to create JWT programmatically and have far superior control available.

Apologies for stepping in unannounced. Let me know if I can help out any other way.

Not at all, I think Litestream is an amazing tool and should be available in a NATS centric pipeline.

stumct-sa · 2024-10-14T19:10:58Z

I was looking through the litestream docs to see if it supported NATS then came across this PR. Would be great to see this get over the line.

I agree with the previous comment about the bucket configuration. I would expect to configure my own bucket and just tell litestream which one to use. There is a number of bucket configurations that will be set depending on my use case at the time. I suggest just taking in a bucket name and expecting it to exist.

JWT authentication is my preferred approach but again I wouldn't have litestream be so opinionated. It should support any NATS auth mechanism

delaneyj · 2024-11-14T14:30:29Z

I have been slammed with other projects but I agree with the comments above and will remove the bucket config from this sorry and look for a review

bruth

@delaneyj Took some time to review given our conversation this past week.

bruth · 2024-12-08T19:06:34Z

nats/replica_client.go

+}
+
+func (c *ReplicaClient) Cleanup(ctx context.Context) (err error) {
+	if c.nc == nil {


Suggested change

if c.nc == nil {

if c.nc != nil {

bruth · 2024-12-08T19:09:43Z

nats/replica_client.go

+
+func (c *ReplicaClient) Cleanup(ctx context.Context) (err error) {
+	if c.nc == nil {
+		c.nc.Close()


Suggested change

c.nc.Close()

c.nc.Drain()

// Block until draining is done. There is also a `ClosedHandler`

// callback option for the NATS connection where a channel

// could be used..

for c.nc.IsDraining() {}

bruth · 2024-12-08T19:18:24Z

nats/replica_client.go

+	BucketReplicas int
+	BucketMaxBytes int64
+	BucketTTL      time.Duration


I presume the goal is an out-of-box "no setup required", but for all other backends (based on a quick scan of the docs) require setting up the bucket ahead of time. These particular properties are operational concerns which should, IMO, be handled out-of-band and would be safer. For instance, the replicas would need to change for some reason. When Litestream would startup, an existing configuration could conflict with the remote settings of that bucket and either overwrite it (depending on the setting) or error since it has changed.

I suggest removing these and the requirement would be that a bucket is pre-created and you only need credentials and the bucket name when configuring this backend.

This would also remove the need to have upsertObjectStore.

bruth · 2024-12-08T19:23:38Z

nats/replica_client.go

+}
+
+// Returns a list of available generations. Order is undefined.
+func (c *ReplicaClient) Generations(ctx context.Context) (_ []string, err error) {


I did not peak at the other backend implementations, so if this is the idiomatic expectation in the codebase then 👍. However, at least for this backend implementation is a bit more difficult to read which values are being returned at times across some of the methods since there is scoped explicit assignment of err in particular vs. implicit.

Sorry, I didn't define what "it" is in the first sentence.. named return values.

bruth · 2024-12-08T19:30:36Z

nats/replica_client.go

+	}
+
+	b := buf.Bytes()
+	natsInfo, err := objectStore.PutBytes(ctx, filename, b)


Could use Put which takes an io.Reader directly.

bruth · 2024-12-08T19:35:42Z

cmd/litestream/main.go

+	JWT            string `yaml:"jwt"`
+	Seed           string `yaml:"seed"`


All auth methods should be supported. If an additional dependency is welcome, this package handles all the options as well as looking up the nats context profile. https://pkg.go.dev/github.com/nats-io/jsm.go/natscontext

bruth · 2024-12-08T20:40:08Z

Annnd.. I just realized I was a bit redundant with the other commentary 🤦. Apologies folks..

initial commit

008dbab

benbjohnson requested changes Aug 13, 2024

View reviewed changes

delaneyj added 2 commits August 22, 2024 07:40

add nats to cli

eff24b7

fixes from PR review

f9898e4

delaneyj requested a review from benbjohnson August 22, 2024 16:42

bruth reviewed Dec 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NATS replica client #596

Add NATS replica client #596

delaneyj commented Jul 31, 2024

benbjohnson left a comment

benbjohnson Aug 13, 2024

delaneyj Aug 22, 2024 •

edited

Loading

betamos commented Aug 29, 2024 •

edited

Loading

delaneyj commented Aug 29, 2024

stumct-sa commented Oct 14, 2024

delaneyj commented Nov 14, 2024

bruth left a comment

bruth Dec 8, 2024

bruth Dec 8, 2024

bruth Dec 8, 2024

bruth Dec 8, 2024

bruth Dec 8, 2024

bruth Dec 8, 2024

bruth Dec 8, 2024

bruth commented Dec 8, 2024

-		c.nc.Close()
+		c.nc.Drain()
+		// Block until draining is done. There is also a `ClosedHandler`
+		// callback option for the NATS connection where a channel
+		// could be used..
+		for c.nc.IsDraining() {}

Add NATS replica client #596

Are you sure you want to change the base?

Add NATS replica client #596

Conversation

delaneyj commented Jul 31, 2024

benbjohnson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delaneyj Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

betamos commented Aug 29, 2024 • edited Loading

delaneyj commented Aug 29, 2024

stumct-sa commented Oct 14, 2024

delaneyj commented Nov 14, 2024

bruth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruth commented Dec 8, 2024

delaneyj Aug 22, 2024 •

edited

Loading

betamos commented Aug 29, 2024 •

edited

Loading