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 mirror #391

Merged
merged 4 commits into from Oct 31, 2022
Merged

KV mirror #391

merged 4 commits into from Oct 31, 2022

Conversation

aricart
Copy link
Member

@aricart aricart commented Oct 27, 2022

No description provided.

@aricart aricart requested a review from kozlovic October 27, 2022 22:25
@aricart aricart temporarily deployed to CI October 28, 2022 13:39 Inactive
@aricart aricart marked this pull request as ready for review October 28, 2022 14:52
@aricart aricart changed the title [WIP] KV mirror KV mirror Oct 28, 2022
@@ -83,6 +109,9 @@ export class StreamAPIImpl extends BaseApiClient implements StreamAPI {
validateStreamName(name);
const old = await this.info(name);
const update = Object.assign(old.config, cfg);
update.mirror = convertStreamSourceDomain(update.mirror);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure updating a stream and changing the mirror/source actually works (in that I don't think the server supports that), so maybe the reason why it is only on "AddStream" in the go client?

Copy link
Member Author

@aricart aricart Oct 28, 2022

Choose a reason for hiding this comment

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

it should be a noop regardless - because it has to have "domain" set which won't be, so it is ignored. I just wanted to trap it for uniformity in case it updates later.

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

… to, to prevent a lookup of a stream by subject.

[FEAT] added support for KVs to be accessed from a mirror
@aricart aricart temporarily deployed to CI October 31, 2022 16:32 Inactive
@aricart
Copy link
Member Author

aricart commented Oct 31, 2022

@kozlovic I added the additional changes required by the client to support history, watch, keys.

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

@aricart aricart merged commit fbdf997 into main Oct 31, 2022
@aricart aricart deleted the kv-mirror branch October 31, 2022 18:03
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