-
Notifications
You must be signed in to change notification settings - Fork 694
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
[ADDED] Support for stream subject transform #1200
Conversation
Update go test_test.mod
c55648a
to
c438530
Compare
There was a problem hiding this 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 👍
Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
👍 |
* Adds support for stream subject transform Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
* Adds support for stream subject transform Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
* Adds support for stream subject transform Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
* Adds support for stream subject transform Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
* Adds support for stream subject transform Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
* Adds support for stream subject transform Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
* Adds support for stream subject transform Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
* Adds support for stream subject transform Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
* Adds support for stream subject transform Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
* Adds support for stream subject transform Co-authored-by: Piotr Piotrowski <piotr@synadia.com>
Adds support for all stream subject transform functionalities and thereby enabled KV bucket sourcing