Support for new stream subject transform functionalities #695

jnmoyne · 2023-02-06T17:51:07Z

Support for the new stream subject transform functionalities:

subject tranform for stream sources
subject tranform for the stream input
sourcing for KV buckets

ripienaar

Let some small comments, I need to see how this looks to really review the UX changes, will look fridayish.

cli/stream_command.go

cli/server_mapping_command.go

ripienaar

some small comments, need more time to run it and see how the UX feels

cli/kv_command.go

cli/stream_command.go

ripienaar · 2023-02-08T12:31:21Z

If you update to latest fisk release I think it will now do some dynamic column width adjustment and on larger displays no more wrapping of flags such that we have 10 more chars to play with for long flags.

Update to that lets see how it looks

ripienaar

Very close now, this is amazing stuff really love it

cli/stream_command.go

ripienaar · 2023-02-17T10:19:29Z

I have one general usability comment:

Given a stream with subjects js.in.test that gets transformed into js.test the js.Subscribe() based flows that looks up a stream by its subjects will not find any stream holding “js.test” messages which leads to some wtfs over and above the wtfs that API already causes.

I think probably the stream should be found by the fact that it holds those messages? Probably a server thing and maybe not fixable but just a weird UX thing I noticed that we might think about

jnmoyne · 2023-02-17T18:21:00Z

About the general usability comment: yes the current js.Subscribe() is a bit broken/too limited, at least the part that tries to find the stream from the subject name being subscribed to. It all comes from the initial JS client API design which (understandably but erroneously) assumes there's a one-to-one link between the subjects a stream currently listens on and the subjects of the messages stored in that stream. That one-to-one hard link was there initially when there was no mirroring/sourcing and you could not modify existing stream attributes, but stopped being there when those features were introduced, and now stream subject transforms is really making it even more obvious that the link between what a stream currently happens to be listening on and the subjects of messages in the streams is no longer there.

Stream subject transform is a bit the final nail in that coffin, but trying to find the stream from just the subject you subscribe to is fundamentally wrong now, even if you tried to look at the subjects of the messages in the stream (can't just look at the current transform as that also can change over the lifespan of the stream) you can have more than one stream with messages on a particular subject and there's no way the subscribe would be able to know which of those streams you actually want to consume from.

The proper/actual solution to this is already in the works: the JS simplification effort where the JS API no longer try to find the stream from the subject you subscribe to but rather the new simplified JS API requires you explicitly pass the stream name to create a consumer on it.

ripienaar

Left a few comments, I see some earlier ones also still outstanding, looking good!

cli/stream_command.go

ripienaar

Looking good, just remove all the redundant humanize calls.

Only time we need them now is with AddRowf()

cli/kv_command.go

ripienaar · 2023-08-08T08:00:10Z

@jnmoyne how are we on this? Can you rebase? People desperate for the CLI release so I want to get this one in.

jnmoyne · 2023-08-08T21:41:33Z

I'd also like to get this one finally merged :). Actually rebased yet again 5 days ago but yeah needed another one since, which is done now.

jnmoyne · 2023-08-08T21:58:37Z

The status is that it's still good to go. Since that PR was created there's been additional functionality (i.e. multi subject support) in stream sources and mirroring which this doesn't cover as I'd rather create a new PR for those. If you'd prefer I did those changes in new commits to this branch instead let me know.

ripienaar · 2023-08-09T09:38:08Z

The status is that it's still good to go. Since that PR was created there's been additional functionality (i.e. multi subject support) in stream sources and mirroring which this doesn't cover as I'd rather create a new PR for those. If you'd prefer I did those changes in new commits to this branch instead let me know.

Unfortunately looks like latest clean up introduced a number of formatting bugs and code layout changes etc, left some comments there.

Is the server work in 2.10 now done and merged completely?

I prefer smaller PRs for sure. so do not want to add to this. Even this I'd have done as many PRs rather than a huge singe PR :)

cli/consumer_command.go

cli/kv_command.go

cli/stream_command.go

Remove redundant humanize Make a few things consistent and cleanup a couple redundancies from the merge Implement PR feedback Source details made more compact Make test check for both arguments being non-null to improve readability Adds lag/last seen/external/error for sources in KV info improve stream report Rebase merge from main

Add empty lines around control statements to improve readability Update go.mod for nats.go to the 2.10.0 branch current latest commit

ripienaar

small changes left

cli/kv_command.go

cli/stream_command.go

Clean up republish flag Simplify and fix config checking for republish Add empty lines around control statements to improve readability Update go.mod for nats.go to be able to build

ripienaar

very very close now :)

cli/stream_command.go

Improved checking of republish and transform arguments being passed in create, copy and edit

ripienaar

LGTM

jnmoyne requested a review from ripienaar February 6, 2023 17:51

ripienaar requested changes Feb 6, 2023

View reviewed changes

cli/stream_command.go Outdated Show resolved Hide resolved

cli/stream_command.go Outdated Show resolved Hide resolved

cli/server_mapping_command.go Outdated Show resolved Hide resolved

jnmoyne force-pushed the jnm/streamsourcetransform branch 3 times, most recently from 901e76e to 7bd478a Compare February 7, 2023 00:01

ripienaar requested changes Feb 7, 2023

View reviewed changes

ripienaar requested changes Feb 17, 2023

View reviewed changes

jnmoyne force-pushed the jnm/streamsourcetransform branch from 2af2c5d to ef7a92f Compare May 9, 2023 19:36

ripienaar requested changes May 10, 2023

View reviewed changes

cli/stream_command.go Show resolved Hide resolved

cli/stream_command.go Outdated Show resolved Hide resolved

cli/stream_command.go Outdated Show resolved Hide resolved

cli/stream_command.go Outdated Show resolved Hide resolved

cli/stream_command.go Outdated Show resolved Hide resolved

jnmoyne force-pushed the jnm/streamsourcetransform branch from ef7a92f to a0178a4 Compare May 16, 2023 00:09

ripienaar requested changes May 16, 2023

View reviewed changes

cli/kv_command.go Outdated Show resolved Hide resolved

cli/kv_command.go Outdated Show resolved Hide resolved

jnmoyne force-pushed the jnm/streamsourcetransform branch from b7685c5 to 43885f0 Compare June 13, 2023 21:47

jnmoyne force-pushed the jnm/streamsourcetransform branch from c7c6121 to a3e7441 Compare June 29, 2023 00:24

jnmoyne force-pushed the jnm/streamsourcetransform branch from 3bff9a0 to d3a2e1f Compare August 4, 2023 00:27

jnmoyne force-pushed the jnm/streamsourcetransform branch from 0cb75cd to 2e94251 Compare August 8, 2023 21:39

ripienaar requested changes Aug 10, 2023

View reviewed changes

jnmoyne added 3 commits August 10, 2023 02:54

Cleanup unused

9dd9a94

PR feedback

e0237b0

Add empty lines around control statements to improve readability Update go.mod for nats.go to the 2.10.0 branch current latest commit

jnmoyne force-pushed the jnm/streamsourcetransform branch from a9d446b to e0237b0 Compare August 10, 2023 09:55

jnmoyne requested a review from ripienaar August 10, 2023 09:56

ripienaar requested changes Aug 10, 2023

View reviewed changes

cli/kv_command.go Outdated Show resolved Hide resolved

cli/stream_command.go Outdated Show resolved Hide resolved

cli/stream_command.go Outdated Show resolved Hide resolved

cli/stream_command.go Outdated Show resolved Hide resolved

cli/stream_command.go Outdated Show resolved Hide resolved

Missing humanizes to IBytes

4ba45ec

Clean up republish flag Simplify and fix config checking for republish Add empty lines around control statements to improve readability Update go.mod for nats.go to be able to build

ripienaar requested changes Aug 11, 2023

View reviewed changes

cli/stream_command.go Outdated Show resolved Hide resolved

cli/stream_command.go Outdated Show resolved Hide resolved

cli/stream_command.go Show resolved Hide resolved

jnmoyne added 4 commits August 10, 2023 23:58

Fix stream copy

0f60ce9

Improved checking of republish and transform arguments being passed in create, copy and edit

Mention --headers-only in the error message

9d4d9b5

Format

e462d19

Humanize

48b8e9d

ripienaar approved these changes Aug 11, 2023

View reviewed changes

jnmoyne merged commit 3edc322 into main Aug 11, 2023
4 checks passed

jnmoyne deleted the jnm/streamsourcetransform branch August 11, 2023 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for new stream subject transform functionalities #695

Support for new stream subject transform functionalities #695

jnmoyne commented Feb 6, 2023

ripienaar left a comment

ripienaar left a comment

ripienaar commented Feb 8, 2023

ripienaar left a comment

ripienaar commented Feb 17, 2023

jnmoyne commented Feb 17, 2023

ripienaar left a comment

ripienaar left a comment

ripienaar commented Aug 8, 2023

jnmoyne commented Aug 8, 2023

jnmoyne commented Aug 8, 2023 •

edited

Loading

ripienaar commented Aug 9, 2023

ripienaar left a comment

ripienaar left a comment

ripienaar left a comment

Support for new stream subject transform functionalities #695

Support for new stream subject transform functionalities #695

Conversation

jnmoyne commented Feb 6, 2023

ripienaar left a comment

Choose a reason for hiding this comment

ripienaar left a comment

Choose a reason for hiding this comment

ripienaar commented Feb 8, 2023

ripienaar left a comment

Choose a reason for hiding this comment

ripienaar commented Feb 17, 2023

jnmoyne commented Feb 17, 2023

ripienaar left a comment

Choose a reason for hiding this comment

ripienaar left a comment

Choose a reason for hiding this comment

ripienaar commented Aug 8, 2023

jnmoyne commented Aug 8, 2023

jnmoyne commented Aug 8, 2023 • edited Loading

ripienaar commented Aug 9, 2023

ripienaar left a comment

Choose a reason for hiding this comment

ripienaar left a comment

Choose a reason for hiding this comment

ripienaar left a comment

Choose a reason for hiding this comment

jnmoyne commented Aug 8, 2023 •

edited

Loading