-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[ADDED] Publish header "Nats-Expected-Last-Subject-Sequence-Subject" #5281
[ADDED] Publish header "Nats-Expected-Last-Subject-Sequence-Subject" #5281
Conversation
server/jetstream_test.go
Outdated
|
||
// Constrain the sequence restriction to a specific subject | ||
// e.g. "kv.1.*" for kv.1.foo, kv.1.bar, kv.1.baz; kv.2.* for kv.2.foo, kv.2.baz; kv.3.* for kv.3.bar | ||
filterSubject := fmt.Sprintf("%s.*", subj[:strings.LastIndex(subj, ".")]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test would be a bit more clear if you lifted the matching subject as a parameter. This would also enable expansion for more nested subjects if/when is relevant.
f5d181d
to
e9b1b36
Compare
Use case aside, the one concern I have is that The question is whether this lands and the latency is not to folks expectations. @cchamplin Have you created a realistic app/test case against this to assess if the writes/second are sufficient for your needs? I did some raw benchmarking against the filestore API to assess, but will want to try it out in a more practical test case. |
@bruth We have not created the type of test case you described. We aren't quite mature enough yet to know if this will negatively impact our performance in a way that matters, we suspect not, we don't have that many subjects that would be matched by our sequence subject pattern. Ultimately though we'd be willing to take a minor performance hit on publishing to unlock this functionality, especially if it's something that could be optimized in the future. |
@neilalexander @ripienaar I advocate for this coming into 2.11. There is no new risk. For those who use it, the performance profile would be equivalent to any other code path that hits All that said, would like to hear your thoughts. |
Yeah seems like a useful feature, for sure 2.11 at the earliest |
This change adds a new header "Nats-Expected-Last-Subject-Sequence-Subject" when when paired with "Nats-Expected-Last-Subject-Sequence" allows publishers to customize the subject used when the server enforces "Nats-Expected-Last-Subject-Sequence". Publishers can specify a alternative subject to be used that includes wildcards. Resolves nats-io#5280 Signed-off-by: Caleb Champlin <[email protected]>
e9b1b36
to
e13afcf
Compare
@derekcollison this is ready for final review and merge for 2.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This change adds a new header
"Nats-Expected-Last-Subject-Sequence-Subject" when when paired with "Nats-Expected-Last-Subject-Sequence" allows publishers to customize the subject used when the server enforces
"Nats-Expected-Last-Subject-Sequence". Publishers can specify a alternative subject to be used that includes wildcards.
Resolves #5280
Signed-off-by: Caleb Champlin [email protected]