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

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Jun 15, 2022

Fixes #15.

Before running a test case, the sdk-go does:

  • Waits for the network sidecar to initialize the network.
  • Signals being initialized to the global count.
  • Signals being initialized to the group count.
  • Records the global sequence number and the group sequence number as a message.

https://github.com/testground/sdk-go/blob/49c90fa754052018b70c63d87b7f1d37f6080a78/run/init_context.go#L43-L65

With this commit the sdk-rust follows the same steps. In addition it not only
signals but also waits for all instances and the instances in the group to
being initialized.

Opening as draft for now due to small TODOs left. Already interested in what you think @laurentsenta @ackintosh @SionoiS.

@mxinden mxinden requested a review from laurentsenta June 15, 2022 13:42
@@ -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.

👍

Before running a test case, the sdk-go does:

- Waits for the network sidecar to initialize the network.
- Signals *being initialized* to the global count.
- Signals *being initialized* to the group count.
- Records the global sequence number and the group sequence number as a message.

With this commit the sdk-rust follows the same steps. In addition it not only
signals but also waits for all instances and the instances in the group to
*being initialized*.
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.

@mxinden mxinden marked this pull request as ready for review June 22, 2022 03:40
@mxinden
Copy link
Member Author

mxinden commented Jun 22, 2022

@mxinden mxinden merged commit 2e0f392 into testground:master Jun 22, 2022
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.

Implement init proceedure
4 participants