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

[ADDED] KeyValue: RePublish option in KeyValueConfiguration #1031

Merged
merged 2 commits into from
Aug 3, 2022

Conversation

kozlovic
Copy link
Member

@kozlovic kozlovic commented Aug 3, 2022

Ability to create a KV with the re-publish feature.

Signed-off-by: Ivan Kozlovic [email protected]

Ability to create a KV with the re-publish feature.

Signed-off-by: Ivan Kozlovic <[email protected]>
@kozlovic
Copy link
Member Author

kozlovic commented Aug 3, 2022

@bruth Wanted to add you as a reviewer, but for some reason I could not find you in the list. Maybe you need to be added into some GH lists.

Copy link
Contributor

@codegangsta codegangsta left a comment

Choose a reason for hiding this comment

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

Nice and easy. LGTM 👍

@coveralls
Copy link

coveralls commented Aug 3, 2022

Coverage Status

Coverage decreased (-0.1%) to 85.559% when pulling 92ea51f on kv_republish into ec49000 on main.

kv.go Show resolved Hide resolved
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

@kozlovic
Copy link
Member Author

kozlovic commented Aug 3, 2022

@codegangsta @derekcollison I think that the stream cannot be updated with RePublish, meaning it may not reject the update per-se (as it did with pull consumer MaxWaiting), but it is actually not updating the stream and therefore the feature is not enabled. For this PR maybe I keep it that it would fail if the stream is originally not created with RePublish (don't try to auto-update). Will then need to add a test in server to demonstrate the issue, or @derekcollison if you think that it should be supported or should work, I may have done something wrong in my client test...

@derekcollison
Copy link
Member

I am ok with rejecting, just need to formalize the rejection in the server code. If need be we can add update support at a later time.

@kozlovic
Copy link
Member Author

kozlovic commented Aug 3, 2022

Ok, so will update this PR with test that verifies that 2nd create with republish fails as expected, then will go to server to add test that ensure that we reject update properly (started to work on a test and update is not rejected, but republish update is clearly not applied).

@derekcollison
Copy link
Member

Perfect thanks.

This cannot be updated in the server at the moment, so if user
calls CreateKeyValue() with a config that does not have RePublish
and then call again on the same bucket with RePublish set, the
user will get an error. Added a test that shows that this is the case.

Signed-off-by: Ivan Kozlovic <[email protected]>
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.

4 participants