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 prefix handling #239

Merged
merged 7 commits into from Jan 18, 2022
Merged

KV prefix handling #239

merged 7 commits into from Jan 18, 2022

Conversation

aricart
Copy link
Member

@aricart aricart commented Dec 23, 2021

[FEAT] Properly support KV across accounts:

a sample server configuration:

accounts: {
      A: {
        jetstream: true,
        users: [{ user: "a", password: "a" }],
        exports: [
          { service: "$JS.API.>" },
          { service: "$KV.>" },
          { stream: "forb.>" },
        ],
      },
      B: {
        users: [{ user: "b", password: "b" }],
        imports: [
          { service: { subject: "$JS.API.>", account: "A" }, to: "froma.>" },
          { service: { subject: "$KV.>", account: "A" }, to: "froma.$KV.>" },
          { stream: { subject: "forb.>", account: "A" } },
        ],
      },
    },

In the above:

  • A exports:
    • Its JetStream API - required to use KV
    • A service all subjects relating to KV
    • A stream
  • B imports:
    • The JetStream API
    • The KV subjects, using a prefix
    • The prefixed stream

In ordered to access the KV, the client in B:

  • Creates a connection setting the inboxPrefix to forb (this will force NATS to know that subscription interest on forb.> is required by account B and allow the ordered push consumer to work for watches and history.
  • Using the same connection, the client creates a JetStreamClient, setting the option apiPrefix to froma, JetStreamManager requests to setup the KV, etc, will now flow to the account's A JetStream.
  • KVs created with that JetStream will now properly target buckets in A.

[CHANGE] internal Bucket.create now requires a JetStreamClient instead of a NatsConnection - this allows propagation of the JetStream options to the KV, which enables prefixes (from the NatsConnection) and apiPrefix from the JetStreamClient to be properly honored by the Kv.

[FIX] consumer options ignored inboxPrefix connection option when auto-generating an inbox for a deliverTo, this, in turn, prevented things like watch/history from working on across accounts because in order to circumvent consumer issues across accounts a stream must be exported, and subscriber must use this prefix in order to receive messages. Possibly this is incorrect but requires current consumer across account strategy to be changed to pull (or at least support ordered pull consumers)

[FIX] when re-creating an ordered consumer, the API prefix was incorrectly used as an inbox prefix.

fixed use of createInbox where intended inboxPrefix was incorrectly set to the apiPrefix
…am prefixes are honored correctly by the KV, when specified on the jetstream options
Copy link

@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.

Just a few questions and a name change

@@ -398,7 +398,7 @@ export class JetStreamClientImpl extends BaseApiClient
ErrorCode.ApiError,
);
}
jsi.config.deliver_subject = createInbox();
jsi.config.deliver_subject = createInbox(this.nc.options.inboxPrefix);

Choose a reason for hiding this comment

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

so this.nc. does not have a create inbox function that makes use of the inbox Prefix automatically?
As is the case in go.

Copy link
Member Author

Choose a reason for hiding this comment

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

no it doesn't - it is just a function. But anywhere it is called in the lib it is honored.

nats-base-client/kv.ts Show resolved Hide resolved
nats-base-client/kv.ts Outdated Show resolved Hide resolved
…ally (subject in the stream) to fullKeyName as per review.
@aricart aricart temporarily deployed to CI December 23, 2021 18:25 Inactive
Copy link

@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

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.

None yet

2 participants