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

Document actor system #1748

Merged
merged 7 commits into from
Jul 18, 2023
Merged

Conversation

pquentin
Copy link
Member

@pquentin pquentin added :Docs Changes to the documentation :internal Changes for internal, undocumented features: e.g. experimental, release scripts labels Jul 12, 2023
@pquentin pquentin added this to the 2.8.1 milestone Jul 12, 2023
@pquentin pquentin self-assigned this Jul 12, 2023
Copy link
Member

@inqueue inqueue left a comment

Choose a reason for hiding this comment

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

Thank you for this. I left some minor suggestions.

docs/architecture/actor_system.md Outdated Show resolved Hide resolved
docs/architecture/actor_system.md Outdated Show resolved Hide resolved
docs/architecture/actor_system.md Outdated Show resolved Hide resolved
docs/architecture/actor_system.md Outdated Show resolved Hide resolved
Co-authored-by: Jason Bryan <jbryan@elastic.co>
Co-authored-by: Brad Deam <54515790+b-deam@users.noreply.github.com>
@pquentin pquentin requested review from b-deam and inqueue July 13, 2023 02:16
Copy link
Contributor

@gbanasiak gbanasiak left a comment

Choose a reason for hiding this comment

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

Many thanks! This helps in building the Rally mental map.

I went through PrepareBenchmark so far. A few remarks, a wish list for later mostly:

  • For consistency, TrackPreparator should be renamed to TrackPreparationActor in the charts as that's the name of the class I think.
  • It would be great to include 10k feet description of what each component is doing, e.g. 1) TrackPreparationActor loads a track and track plugins (custom extensions such as parameter sources and runners), gets a list of track processors (TrackProcessor interface) from TrackProcessorRegistry and stores them in a queue. Then it sends each processor for execution to TaskExecutionActor. 2) TackExecutionActor executes each processor in a background task and verifies its status periodically. Once task completes, it reports its readiness with WorkComplete. Not sure what level of details makes sense here - too much is just a repetition of code, too little forces us to go through the flow again if we revisit this after enough time.
  • It would be nice to explain how individual components grow as the scale of the system increases, e.g. TrackPreparationActor set grows with the number of load driver hosts, while TaskExecutionActor set can grow with the number of cores declared per load driver.

docs/architecture/actor_system.md Outdated Show resolved Hide resolved

At its heart, Rally is a distributed system. It has been designed that way to
allow using multiple load drivers in the same benchmark, to ensure that Rally
is never a bottleneck. In the vast majority of cases, using a powerful load
Copy link
Contributor

@dliappis dliappis Jul 13, 2023

Choose a reason for hiding this comment

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

Another reason (the first one, IIRC) is that distributing Rally also provides an easy way to handle multiple target hosts and ensure that they all build/install/start the required ES version via the race subcommand (this is for example the mode used on the Hetzner nightly benchmarks).
Later on we got the dedicated/decoupled esrally build/install/start/stop subcommands (which we use e.g. with cloud based benchmarks) and these are decoupled from the actor system.

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 current formulation isn't correct then, however my understanding is that this is achievable without a full-fledged actor system, while we could easily end up re-implementing Thespian badly to support multiple load drivers.

Copy link
Contributor

Choose a reason for hiding this comment

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

however my understanding is that this is achievable without a full-fledged actor system, while we could easily end up re-implementing Thespian badly to support multiple load drivers.

I mostly agree. I don't think the risk of supporting >1 load drivers without the actor system would necessarily result in re-implementing Thespian, but as always, the devil is in the details.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've changed the wording slightly in ac1aec8.

@pquentin
Copy link
Member Author

I went through PrepareBenchmark so far. A few remarks, a wish list for later mostly:

I fixed TrackPreparationActor, would you like contributing your other fixes to this branch or in another pull request?

@gbanasiak
Copy link
Contributor

@dliappis @inqueue I've adjusted description based on the previous discussion and also updated diagrams for StartBenchmark section while I was reviewing the code myself. I will continue with extra additions in this doc in other PRs, if needed. PTAL.

Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

This LGTM -- so great to see these internal aspects of Rally documented.

One thing I wanted to ask is how come this isn't living under the official docs e.g. in the Reference Documentation or Additional Information sections? (is it because Mermaid isn't available?)

@gbanasiak
Copy link
Contributor

One thing I wanted to ask is how come this isn't living under the official docs e.g. in the Reference Documentation or Additional Information sections? (is it because Mermaid isn't available?)

It should be possible to use Mermaid with Sphinx but it would require additional package, see https://github.com/mgaitan/sphinxcontrib-mermaid. That's work in progress. If/when this matures we can include this in official documentation.

@gbanasiak gbanasiak merged commit e39834d into elastic:master Jul 18, 2023
@gbanasiak gbanasiak modified the milestones: 2.8.1, 2.9.0 Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Docs Changes to the documentation :internal Changes for internal, undocumented features: e.g. experimental, release scripts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants