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

[ADDED] Support for stream subject transform #1200

Merged
merged 9 commits into from
Feb 6, 2023
Merged

[ADDED] Support for stream subject transform #1200

merged 9 commits into from
Feb 6, 2023

Conversation

jnmoyne
Copy link
Contributor

@jnmoyne jnmoyne commented Jan 28, 2023

Adds support for all stream subject transform functionalities and thereby enabled KV bucket sourcing

@coveralls
Copy link

coveralls commented Jan 28, 2023 •
edited

Coverage Status

Coverage: 85.577% (+0.1%) from 85.467% when pulling 1435b8e on jnm/streamsourcetransform into a301969 on main.

piotrpio
piotrpio reviewed Feb 1, 2023
View reviewed changes
Copy link
Collaborator

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

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

Just some nits, other than that it looks good 👍

jsm.go Outdated Show resolved Hide resolved
jsm.go Outdated Show resolved Hide resolved
kv.go Outdated Show resolved Hide resolved
kv.go Outdated Show resolved Hide resolved
kv_test.go Outdated Show resolved Hide resolved
js_test.go Show resolved Hide resolved
js_test.go Outdated Show resolved Hide resolved
jnmoyne and others added 6 commits February 1, 2023 12:14
@jnmoyne @piotrpio
Fix typo
e20dcf8 
Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
@jnmoyne @piotrpio
Fix typo
e39179e 
Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
@jnmoyne @piotrpio
re-use existing template definition
7be647c 
Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
@jnmoyne @piotrpio
re-use existing template definition
41ed469 
Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
@jnmoyne
Cleanup
ebb3e46
@jnmoyne
shorten wait time in test
a19a29a
piotrpio
piotrpio approved these changes Feb 3, 2023
View reviewed changes
Copy link
Collaborator

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

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

LGTM

wallyqs
wallyqs reviewed Feb 3, 2023
View reviewed changes
kv_test.go Outdated
}

// Wait half a second to make sure it has time to populate the stream from it's sources
time.Sleep(100 * time.Millisecond)
Copy link
Member

@wallyqs wallyqs Feb 3, 2023 •
edited

Choose a reason for hiding this comment

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

This looks flaky... why not just wrap below code with the kvC.Get with some retries until they work? Or wrap with a block js.StreamInfo that it has messages for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not flaky, just a question of timing. Without it, the test passes like 99% of the time but I did see it fail once. IMHO it's expected as there is a small delay between the stream creation call returning and the stream being populated from it's sources, so I added the sleep to give the stream a little bit of time to come up and get populated with the two messages from the 2 sources and haven't seen it fail since. I actually lowered it further to 20 ms and it still passes every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did implement something like you suggest of checking the number of messages and waiting until it reaches 2 and then doing the gets - see the last commit.
And what happens is that the first size check always returns 0 so it does the sleep of 20ms then the next time around the stream is populated and it goes on with the gets, so the tests with this extra check before sleep always runs the same amount of time as with just the sleep.

wallyqs
wallyqs reviewed Feb 3, 2023
View reviewed changes
js_test.go Outdated Show resolved Hide resolved
@wallyqs wallyqs merged commit c860828 into main Feb 6, 2023
@jnmoyne jnmoyne deleted the jnm/streamsourcetransform branch February 6, 2023 01:06
@jnmoyne
Copy link
Contributor Author

jnmoyne commented Feb 6, 2023

👍

@wallyqs wallyqs mentioned this pull request Feb 6, 2023
Merged
piotrpio added a commit that referenced this pull request Feb 24, 2023
@piotrpio
Revert "[ADDED] Support for stream subject transform (#1200)"
5c904ce 
This reverts commit c860828.
piotrpio added a commit that referenced this pull request Feb 24, 2023
@jnmoyne @piotrpio
[ADDED] Support for stream subject transform (#1200)
61c6e0c 
* Adds support for stream subject transform

Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
@piotrpio piotrpio mentioned this pull request Feb 24, 2023
Merged
piotrpio added a commit that referenced this pull request Feb 24, 2023
@jnmoyne @piotrpio
[ADDED] Support for stream subject transform (#1200)
89abbf8 
* Adds support for stream subject transform

Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio added a commit that referenced this pull request Feb 24, 2023
@piotrpio
Revert "[ADDED] Support for stream subject transform" (#1220)
c86a1a1 
* Revert "[ADDED] Support for stream subject transform (#1200)"

This reverts commit c860828.

* Revert "js: mirror test updates"

This reverts commit 424de47.
@jnmoyne jnmoyne mentioned this pull request Mar 16, 2023
Open
15 tasks
piotrpio added a commit that referenced this pull request Jun 14, 2023
@jnmoyne @piotrpio
[ADDED] Support for stream subject transform (#1200)
aa6cc9e 
* Adds support for stream subject transform

Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
jnmoyne added a commit that referenced this pull request Aug 9, 2023
@jnmoyne @piotrpio
[ADDED] Support for stream subject transform (#1200)
c101340 
* Adds support for stream subject transform

Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio added a commit that referenced this pull request Aug 26, 2023
@jnmoyne @piotrpio
[ADDED] Support for stream subject transform (#1200)
c1c2f70 
* Adds support for stream subject transform

Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio added a commit that referenced this pull request Sep 1, 2023
@jnmoyne @piotrpio
[ADDED] Support for stream subject transform (#1200)
33fb7d3 
* Adds support for stream subject transform

Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio added a commit that referenced this pull request Sep 1, 2023
@jnmoyne @piotrpio
[ADDED] Support for stream subject transform (#1200)
c9a1679 
* Adds support for stream subject transform

Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio added a commit that referenced this pull request Sep 19, 2023
@jnmoyne @piotrpio
[ADDED] Support for stream subject transform (#1200)
fa5faa8 
* Adds support for stream subject transform

Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio added a commit that referenced this pull request Sep 19, 2023
@jnmoyne @piotrpio
[ADDED] Support for stream subject transform (#1200)
e0763a3 
* Adds support for stream subject transform

Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
piotrpio added a commit that referenced this pull request Sep 20, 2023
@jnmoyne @piotrpio
[ADDED] Support for stream subject transform (#1200)
78f3964 
* Adds support for stream subject transform

Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
@piotrpio piotrpio mentioned this pull request Sep 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wallyqs wallyqs wallyqs approved these changes

@piotrpio piotrpio piotrpio approved these changes

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
4 participants
@jnmoyne @coveralls @wallyqs @piotrpio