-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 in ability to get per subject details via StreamInfo. #2833
Conversation
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.
Would make sure that the test has different number of messages per subject...
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, but the test "TestNoRaceJetStreamSlowFilteredInititalPendingAndFirstMsg" is failing in most of the runs for this PR, but passes locally (-count 10) on my machine...
Yep, removed default NumSubjects for StreamInfo since it was also present in FastState which was taking too long and was used to create consumers etc. |
b8e38c4
to
c3d2c2d
Compare
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
server/store.go
Outdated
FirstTime time.Time `json:"first_ts"` | ||
LastSeq uint64 `json:"last_seq"` | ||
LastTime time.Time `json:"last_ts"` | ||
Subjects map[string]uint64 `json:"subjects,omitempty"` |
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.
Maybe time to revisit compressed API responses and acceptible response content negotiation like we have for server reqs.
As beore when we spoke I am not a huge fan of this approach as its inherently limited, think we'll sooner or later need to consider some streaming api responses anyway
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.
On KV for example - if a KV uses a nkey for the base of a subject, this effectively makes a subject 80+ chars in length. If the KV organizes several values by key, that means that for each datum, we have 2+ values, so assuming 1000 entries, we are looking at at least 2000 subjects or 160K payload for this request. With 1M entries (2M subjects) we are looking at 160MB.
When ever the KV API starts, it will have to retrieve 160MB (as the protocol for the KV does a stream info)
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.
This is opt-in, by default you do not get the subject details (histogram).
We have an artificial limit of 100k subjects that if we have more than that we will error and the user can fall back to the watcher impl like we have now.
Also note that maximum payloads are not in play here since its a system generated message.
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.
What about just making getting subjects a JS API call similar to getting consumers or stream names? This way it could be paged.
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.
Indeed, thats what I also suggested when this was first mooted. However paging a dynamic data set like subjects is bound to be unreliable, hence a seperate API call and streaming results would yield a better outcome.
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.
I considered that quite a bit, but decided to go with similar approach to deleted details for a stream info.
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.
Deleted data opt-in is not really something that I think anyone uses, but this data will be used widely. I wouldnt use deleted data as a gold standard for how to implement this
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.
We debated this quite a bit and this is what I think is best solution given all things considered.
beb9e5a
to
8ee6a97
Compare
I added back in the NumSubjects part of StreamInfo. I think this PR is good, please re-review when you have a moment. |
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, but it looks like there are changes in this PR that have already been submitted in different PRs and likely in main already. Also, seems like you have a conflict, so you may want to rebase.
…nally details per subject on how many messages each subject has. This can also be filtered, meaning you can filter out the subjects when asking for details. Signed-off-by: Derek Collison <derek@nats.io>
Signed-off-by: Derek Collison <derek@nats.io>
8ee6a97
to
5da0453
Compare
I have documented the client changes here: |
Note that we went with a simpler approach vs message per result or a paged api. We will place the subject details (just msg count) into the stream info state. Since NATS as a system can limit payloads of any msg entering the system, but the system itself can send any size message to any client, we allow this to proceed. We artificially limit it to 100k, and for items like KV we can fall back to the watcher/ordered consumer pattern as needed.
Signed-off-by: Derek Collison derek@nats.io
/cc @nats-io/core