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

Allow direct get by subject to be all subject based. #3325

Merged
merged 1 commit into from
Aug 3, 2022
Merged

Conversation

derekcollison
Copy link
Member

This avoids marshaling or un-marshaling but also allows subject based permissioning.

Signed-off-by: Derek Collison [email protected]

/cc @nats-io/core

This avoids marshalling or unmarshalling but also allows subject based permissioning.

Signed-off-by: Derek Collison <[email protected]>
Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

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

LGTM for the scope of this pr. But I believe that PURGE/DELETE/MSGGET is worth bringing up separately.

@@ -3333,6 +3372,53 @@ func (mset *stream) processDirectGetRequest(_ *subscription, c *client, _ *Accou
}
}

// This is for direct get by last subject which is part of the subject itself.
func (mset *stream) processDirectGetLastBySubjectRequest(_ *subscription, c *client, _ *Account, subject, reply string, rmsg []byte) {
Copy link
Member

Choose a reason for hiding this comment

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

So what will happen to the other versions of direct get? Meaning as of now, the Go client is using a nats.DirectGet() or nats.DirectGetNext() option with optional sequence number to get a message at a specific sequence number. Will that go away? Since those were added very recently and not yet released, I would like to know if we pull them out, etc..

As for the kv implementation, as I previously said, the KV API has a GetRevision() API that retrieves a key at a given sequence:

	if kv.useDirect {
		_opts[0] = DirectGet()
		opts = _opts[:1]
	}
	if revision == kvLatestRevision {
		m, err = kv.js.GetLastMsg(kv.stream, b.String(), opts...)
	} else {
		m, err = kv.js.GetMsg(kv.stream, revision, opts...)
		// If a sequence was provided, just make sure that the retrieved
		// message subject matches the request.
		if err == nil && m.Subject != b.String() {
			return nil, ErrKeyNotFound
		}
	}

So with the change in the server, I could see how we would modify GetLastMsg() in case of direct to append the subject and not have a payload at all, but what about the other cases and the JSM management get message APIs.

Copy link
Member Author

Choose a reason for hiding this comment

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

The current direct get that takes a request body will continue to work as is today.

I would expect clients to optimize and use this method when they know they are just looking up last value by subject/key as it avoids a marshal and can be subject to security permissions, but the current direct get will continue to work.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, so I read that we should try to optimize the clients GetLastMsg() when nats.DirectGet() option is provided to simply add the subject at the end, and not send a marshal request.

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.

3 participants