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

KV Updating not available. UpdateStream only runs when there is no changes #1538

Closed
MashinaMashina opened this issue Jan 30, 2024 · 3 comments
Assignees
Labels
accepted The defect or proposal as been accepted proposal Enhancement idea or proposal

Comments

@MashinaMashina
Copy link

MashinaMashina commented Jan 30, 2024

Observed behavior

When I use nats.jetstream.CreateKeyValue after changing TTL, I got error "nats: stream name already in use"

Expected behavior

I use nats.jetstream.CreateKeyValue and the TTL will update.

Server and client version

nats-io/nats.go version 1.31.0
nats in docker: nats:2.9-alpine3.18

Host environment

No response

Steps to reproduce

... connect to nats

// no error
_, err := jetstream.CreateKeyValue({
		Bucket:       "kv_name",
		MaxValueSize: 1024,
		TTL:             time.Minute,
		Storage:      nats.FileStorage,
	})

// Expected no error, but got stream name already in use
_, err = jetstream.CreateKeyValue({
		Bucket:       "kv_name",
		MaxValueSize: 1024,
		TTL:             0,
		Storage:      nats.FileStorage,
	})

==================

this behavior because we have this code (https://github.com/nats-io/nats.go/blob/main/kv.go#L489):

if errors.Is(err, ErrStreamNameAlreadyInUse) {
			if si, _ = js.StreamInfo(scfg.Name); si != nil {
				// To compare, make the server's stream info discard
				// policy same than ours.
				si.Config.Discard = scfg.Discard
				// Also need to set allow direct for v2.9.x+
				si.Config.AllowDirect = scfg.AllowDirect
				if reflect.DeepEqual(&si.Config, scfg) {
					si, err = js.UpdateStream(scfg)
				}
			}
		}

In my opinion it must be like this:

if errors.Is(err, ErrStreamNameAlreadyInUse) {
			if si, _ = js.StreamInfo(scfg.Name); si != nil {
				// To compare, make the server's stream info discard
				// policy same than ours.
				si.Config.Discard = scfg.Discard
				// Also need to set allow direct for v2.9.x+
				si.Config.AllowDirect = scfg.AllowDirect
				if reflect.DeepEqual(&si.Config, scfg) {
					err = nil
				} else {
					si, err = js.UpdateStream(scfg)
				}
			}
		}

If configs are equals - do nothing, else - update stream. Now updating only if there are no changes - this is wrong.

@MashinaMashina MashinaMashina added the defect Suspected defect such as a bug or regression label Jan 30, 2024
@ripienaar
Copy link
Contributor

We do need a KV Update function - but maybe make it a specific function rather than overuse this code path. This code path mainly is there to upgrade configuration of old buckets to current internal needs afaik

@piotrpio
Copy link
Collaborator

piotrpio commented Feb 1, 2024

Hello @MashinaMashina, thanks for creating the issue. As @ripienaar said, we'll be adding an UpdateKeyValue() method for that, rather than modifying create. Possibly we'll also add CreateOrUpdate to match what we have for streams and consumers.

@piotrpio piotrpio self-assigned this Feb 1, 2024
@piotrpio piotrpio added proposal Enhancement idea or proposal accepted The defect or proposal as been accepted and removed defect Suspected defect such as a bug or regression labels Feb 1, 2024
@piotrpio
Copy link
Collaborator

We've added UpdateKeyValue and CreateOrUpdateKeyValue methods in a recent PR: #1549

It is now available in release v1.33.1. Thank you for creating the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted The defect or proposal as been accepted proposal Enhancement idea or proposal
Projects
None yet
Development

No branches or pull requests

3 participants