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.

Sign up for GitHub

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

Jump to bottom

Adds pagination to stream info #3454

Merged
merged 1 commit into from Sep 8, 2022
Merged

Adds pagination to stream info #3454

merged 1 commit into from Sep 8, 2022

Conversation

jnmoyne
Copy link
Contributor

@jnmoyne jnmoyne commented Sep 8, 2022 •
edited

  • Link to issue, e.g. Resolves #NNN
  • Documentation added (if applicable)
  • Tests added
  • Branch rebased on top of current main (git pull --rebase origin main)
  • Changes squashed to a single commit (described here)
  • Build is green in Travis CI
  • You have certified that the contribution is your original work and that you license the work to the project under the Apache 2 license

Resolves #

Changes proposed in this pull request:

  • Adds pagination to the JS Stream Info API (backwards compatible) that was previously hard-coded to a max number of 100K subject states

/cc @nats-io/core

derekcollison
derekcollison reviewed Sep 8, 2022
View reviewed changes
server/jetstream_api.go Show resolved Hide resolved
server/jetstream_api.go Outdated Show resolved Hide resolved
server/jetstream_api.go Outdated Show resolved Hide resolved
derekcollison
derekcollison approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM - but let's have Ivan eyeball too. Also feel free to squash.

kozlovic
kozlovic requested changes Sep 8, 2022
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Some changes needed, especially to prevent panic.
Also, it means that all clients would need to be updated to support this pagination options in the StreamInfo() API, right?

server/jetstream_api.go Outdated Show resolved Hide resolved
server/jetstream_api.go Outdated Show resolved Hide resolved
@jnmoyne
Copy link
Contributor Author

jnmoyne commented Sep 8, 2022

Yes, the clients need to be updated to be able to leverage pagination. As soon as this is merged I can create PRs (I already have the coding done) for natscli, jsm.go and nats.go (I can probably have a go at Java as well).
Existing clients still work (these changes are backwards compatible) though obviously then can only get the first 100k subject details

kozlovic
kozlovic requested changes Sep 8, 2022
View reviewed changes
server/jetstream_api.go Outdated Show resolved Hide resolved
server/jetstream_api.go Outdated Show resolved Hide resolved
server/jetstream_api.go Outdated Show resolved Hide resolved
kozlovic
kozlovic approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

LGTM

@kozlovic kozlovic merged commit 6fba6c8 into main Sep 8, 2022
@kozlovic kozlovic deleted the js_stream_info_pagination branch September 8, 2022 18:23
@jnmoyne jnmoyne mentioned this pull request Sep 8, 2022
Closed
jnmoyne added a commit to nats-io/nats.go that referenced this pull request Sep 9, 2022
@jnmoyne
Implements a new jsm function StreamContainedSubjects()` to get the…
31624d1 
… list of (matching) subjects for the messages currently in the stream and the number of messages for each subject, using the pagination API for Stream Info (see nats-io/nats-server#3454)
jnmoyne added a commit to nats-io/nats.go that referenced this pull request Sep 9, 2022
@jnmoyne
Implements a new jsm function StreamContainedSubjects()` to get the…
4d1100a 
… list of (matching) subjects for the messages currently in the stream and the number of messages for each subject, using the pagination API for Stream Info (see nats-io/nats-server#3454)
@jnmoyne jnmoyne mentioned this pull request Sep 9, 2022
Merged
jnmoyne added a commit to nats-io/nats.go that referenced this pull request Sep 13, 2022
@jnmoyne
Implements a new jsm function StreamContainedSubjects()` to get the…
620af9c 
… list of (matching) subjects for the messages currently in the stream and the number of messages for each subject, using the pagination API for Stream Info (see nats-io/nats-server#3454)
jnmoyne added a commit to nats-io/nats.go that referenced this pull request Sep 13, 2022
@jnmoyne
Implements a new jsm function StreamContainedSubjects()` to get the…
ace932a 
… list of (matching) subjects for the messages currently in the stream and the number of messages for each subject, using the pagination API for Stream Info (see nats-io/nats-server#3454)
jnmoyne added a commit to nats-io/nats.go that referenced this pull request Sep 13, 2022
@jnmoyne
Implements a new jsm function StreamContainedSubjects()` to get the…
6a4637b 
… list of (matching) subjects for the messages currently in the stream and the number of messages for each subject, using the pagination API for Stream Info (see nats-io/nats-server#3454)
jnmoyne added a commit to nats-io/nats.go that referenced this pull request Sep 13, 2022
@jnmoyne
Implements a new jsm function StreamContainedSubjects()` to get the…
b753f69 
… list of (matching) subjects for the messages currently in the stream and the number of messages for each subject, using the pagination API for Stream Info (see nats-io/nats-server#3454)
kozlovic pushed a commit to nats-io/nats.go that referenced this pull request Sep 15, 2022
@jnmoyne @kozlovic
[CHANGED] StreamInfo() will now return all subjects when requested
65796fc 
If a subject filter is specified in the StreamInfoRequest{} option,
then all matching subjects will be returned and not be capped to
the server limit of 100,000. It is internally using pagination
that was added in the server PR: nats-io/nats-server#3454
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekcollison derekcollison derekcollison approved these changes

@kozlovic kozlovic kozlovic approved these changes

@matthiashanel matthiashanel Awaiting requested review from matthiashanel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jnmoyne @derekcollison @kozlovic