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

src/client: Implement init proceedure #25

Merged
merged 4 commits into from
Jun 22, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Make `RunParameters::test_group_instance_count` a `u64` to be consistent with
`RunParameters::test_instance_count`. See [PR 26].

- Replace `Client::new` with `Client::new_and_init`, waiting for the network to
initialize, claiming global and group sequence numbers, as well as waiting for
other instances to do the same. Also makes `Client::wait_network_initialized`
private, as it is included in `Client::new_and_init` now. See [PR 25].

[PR 26]: https://github.com/testground/sdk-rust/pull/26
[PR 26]: https://github.com/testground/sdk-rust/pull/25

## [0.3.0]
### Added
Expand Down
2 changes: 1 addition & 1 deletion examples/example.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
let (client, run_parameters) = testground::client::Client::new().await?;
let (client, run_parameters) = testground::client::Client::new_and_init().await?;

client.record_message(format!(
"{}, sdk-rust!",
Expand Down
27 changes: 24 additions & 3 deletions src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,37 @@ pub struct Client {
}

impl Client {
pub async fn new() -> Result<(Self, RunParameters), Box<dyn std::error::Error>> {
pub async fn new_and_init() -> Result<(Self, RunParameters), Box<dyn std::error::Error>> {
let params = RunParameters::try_parse()?;

let (cmd_tx, cmd_rx) = channel(1);

let background = BackgroundTask::new(cmd_rx, params.clone()).await?;
let client = Self { cmd_tx };

tokio::spawn(background.run());

Ok((Self { cmd_tx }, params))
client.wait_network_initialized().await?;

let global_seq_num = client
// Note that the sdk-go only signals, but not waits.
.signal_and_wait("initialized_global", params.test_instance_count)
.await?;

let group_seq_num = client
// Note that the sdk-go only signals, but not waits.
.signal_and_wait(
format!("initialized_group_{}", params.test_group_id),
params.test_group_instance_count as u64,
)
.await?;

client.record_message(format!(
"claimed sequence numbers; global={}, group({})={}",
global_seq_num, params.test_group_id, group_seq_num
));

Ok((client, params))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks for sharing the draft!

A couple of notes:

  • the go sdk doesn't wait in the init, but provides methods to do so
    • I can't see a good reason to need this level of granularity, I guess we'll port this behavior to the go-sdk
  • the go sdk sends the global seq and group seq to the test
    • It might be useful to implement leader/follower quickly. Related discussion
    • I've seen it used in another test as seed value, but that use case seems anecdotal

Copy link
Member Author

Choose a reason for hiding this comment

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

the go sdk sends the global seq and group seq to the test

👍 for exposing it to the test, though I haven't clean way to do so yet. Still want to put more thoughts into it. In my eyes this can happen as a follow up.

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 can't see a good reason to need this level of granularity, I guess we'll port this behavior to the go-sdk

Same here. I can't think of a use-case where one would want to publish but not wait.


/// ```publish``` publishes an item on the supplied topic.
Expand Down Expand Up @@ -133,7 +154,7 @@ impl Client {

/// ```wait_network_initialized``` waits for the sidecar to initialize the network,
/// if the sidecar is enabled.
pub async fn wait_network_initialized(&self) -> Result<(), Error> {
async fn wait_network_initialized(&self) -> Result<(), Error> {
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 can not think of a use-case where one would not call this method at start. Thus I am proposing to include it in new_and_init (see above) and making the method itself private.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't think of a case either! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// Event
let (sender, receiver) = oneshot::channel();

Expand Down