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

Enforce operation order #23

Merged
merged 3 commits into from
Nov 11, 2024
Merged

Enforce operation order #23

merged 3 commits into from
Nov 11, 2024

Conversation

balanza
Copy link
Member

@balanza balanza commented Nov 7, 2024

Enforce the order of execution of the HTTP requests by sorting the fixture set by file name. Optionally, a wait argument can be provided to add a delay between HTTP requests.

Rationale
As this is a tool for replaying events, I expect the ability to define the order in which events should be processed; following the alphabetical order of the files in the directory seems to be the more natural approach.
Furthermore, the wait parameter can slow down the ingestion rate for the receiving service, if needed.

@balanza
Copy link
Member Author

balanza commented Nov 7, 2024

Code formatted with cargo fmt

Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Good!
This makes at least the usage predictable, and we could add maybe some prefixes in the files if we want to have a strict ordering.
Wrote a question about the sleep.

PD: Keep in mind that to use the latest version of photofinish in our CI, we need to create a release and update the version in other projects (we have it hardcoded)

.directories
.iter()
.filter_map(extract_fixtures_from_directory)
.flatten()
.collect();
fixtures_in_directories.sort();
Copy link
Contributor

Choose a reason for hiding this comment

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

dump question: Cannot you pipe the sort after the collect? Or vectors don't allow this?
Maybe using sorted

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, I'm not much into rust so I missed that. I'll try it

Copy link
Member Author

Choose a reason for hiding this comment

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

To use such function I must add a dependency to itertools.
I'd rather accept to have a mutable variable instead.

src/run.rs Outdated
Err(Errored { file, reason }) => {
println!("An error occurred in loading fixture {}: {}", file, reason)
}
}

thread::sleep(std::time::Duration::from_millis(wait));
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we sure about this?
This function is blocking and the docs say it shouldn't be used in async functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are we sure about this?

About the expected behavior: yes, I think we might need a gap between HTTP calls if we want to reduce the pressure on the receiving system (that might not be tuned for receiving a burst of requests).

About using thread::sleep: not sure, for what I red is fine but I will investigate more

Copy link
Member Author

Choose a reason for hiding this comment

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

I found out I'd better use tokio::time::sleep. Done in 8222736

@balanza balanza requested a review from arbulu89 November 11, 2024 09:53
Copy link
Collaborator

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM, nice catch for tokio::sleep 👍

@balanza balanza merged commit b078333 into main Nov 11, 2024
2 checks passed
@balanza balanza deleted the enforce-sequence branch November 11, 2024 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants