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

feat(nats): flag to change persistence of streams #525

Merged

Conversation

markkovari
Copy link
Contributor

Feature or Problem

cli toggle to modify the nats stream persistence between file and memory.

I do seek some guidance regarding testing here. Since we have multiple tests which have the file - default - mode enabled, should I create another suite or how do you think should I add tests for this?

Thank you

Related Issues

#479

Release Information

Consumer Impact

Testing

Unit Test(s)

Acceptance or Integration

Manual Verification

@markkovari markkovari force-pushed the feat/change-persistance-of-streams branch from b9f0d31 to 0582e95 Compare December 17, 2024 22:50
src/main.rs Outdated Show resolved Hide resolved
@ahmedtadde
Copy link
Contributor

ahmedtadde commented Dec 18, 2024

hey @markkovari thank you for this submission! Upon a first look, I only found a spelling issue that needs to be addressed. Otherwise, the changes made should do the trick.

testing wise, I believe we probably want the following

  • ensure all the existing tests work as expected (no regression bugs)... this is basically just running the test suite locally
  • ensure that the flag (parsing) works... as in, when not specified -> expected default value & when specified -> maps to expected values (File, Memory). This may be pedantic though; but a good exercise for a first issue PR. it is up to you.
  • for Memory mode, ensure that there is a file when using File mode and the absence of said file otherwise. This entails figuring out how to find/peek/inspect a JetStream persistence file for a given stream.

@markkovari markkovari changed the title Feat/change persistence of streams feat(nats): flag to change persistence of streams Dec 18, 2024
@markkovari
Copy link
Contributor Author

@ahmedtadde thanks for the elaborate answer, will look into it between two food coma and how the tests are outlined.

Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

Quick request to un-hide the flag, other piece optional but I'm thinking it'll be more clear to have lowercase flag options

src/nats.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@markkovari markkovari force-pushed the feat/change-persistance-of-streams branch from 830dd09 to c18b3d8 Compare December 19, 2024 17:41
Copy link
Member

@brooksmtownsend brooksmtownsend left a comment

Choose a reason for hiding this comment

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

@markkovari this looks good! If there's any test to run, I think there's just two that we can do:

  1. Change one of the e2e tests to use memory backed streams, make sure nothing goes awry there
  2. Manually verify that you can run ./wadm --stream-persistence file, close the wadm instance, then ./wadm --stream-persistence memory without issue.

@markkovari markkovari force-pushed the feat/change-persistance-of-streams branch from cc15e3d to b85a3c6 Compare December 24, 2024 17:23
@markkovari markkovari force-pushed the feat/change-persistance-of-streams branch from b85a3c6 to a552842 Compare December 24, 2024 17:34
@markkovari markkovari marked this pull request as ready for review December 24, 2024 17:35
@markkovari markkovari requested a review from a team as a code owner December 24, 2024 17:35
@brooksmtownsend brooksmtownsend merged commit 40d8b50 into wasmCloud:main Jan 2, 2025
5 checks passed
@markkovari markkovari deleted the feat/change-persistance-of-streams branch January 6, 2025 16:01
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.

3 participants