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

refactor: introduce AppBuilder trait #3941

Closed
wants to merge 2 commits into from

Conversation

zyy17
Copy link
Collaborator

@zyy17 zyy17 commented May 14, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

NOTE: The PR is now in draft mode for discussion. Since the whole PR may be bigger(>1000 LOC), I will separate it into multiple small PRs and share the important abstraction first.

This PR introduces the new trait in the cmd/ crate called AppBuilder. It's a simple abstraction based on the original code that makes our cli structure cleaner.

Based on the original design, we can have the following concept:

  • App: For most scenarios, it's a running service, for example, frontend, metasrv, etc. Also, it can be a cli tool. Each subcommand can be treated as App;

  • AppBuilder: The builder of the App;

  • Options: For building the App, we need Options. For most scenarios, Options can implement Configurable trait;

We can use the AppBuilder to build and run the App:

/// Build the options and app, then start the app.
async fn start(self) -> Result<()> {
    self.build_options()?.build_app().await?.run().await
}

The original implementation has many build() and load_options(), which seems too complicated. If we use AppBuilder, the code can be:

  • Accept the command line argument to create AppBuilder, then build and run the App;
  • Each component(frontend.rs/datanode.rs/metasrv.rs/cli.rs) only needs to implement AppBuilder;

The following is the snippet of the code:

...
#[tokio::main]
async fn main() -> Result<()> {
    ...
    start(Command::parse()).await
}

async fn start(cli: Command) -> Result<()> {
    match cli.subcmd {
        SubCommand::Datanode(cmd) => cmd.new_app_builder(cli.global_options).start().await,
        SubCommand::Frontend(cmd) => cmd.new_app_builder(cli.global_options).start().await,
        SubCommand::Metasrv(cmd) => cmd.new_app_builder(cli.global_options).start().await,
        SubCommand::Standalone(cmd) => cmd.new_app_builder(cli.global_options).start().await,
        SubCommand::Cli(cmd) => cmd.new_app_builder(cli.global_options).start().await,
    }
}
...

For each App, we need to create the AppBuilder struct and implement the trait(most methods are from the original implementation). Take cmd/datanode.rs for example:

...
#[derive(Default)]
pub struct DatanodeAppBuilder {
    command: Option<StartCommand>,
    global_options: GlobalOptions,
    datanode_options: Option<DatanodeOptions>,
}

#[async_trait]
impl AppBuilder for DatanodeAppBuilder {
    fn build_options(mut self) -> Result<Self> { ... }

    async fn build_app(mut self) -> Result<Box<dyn App>> { ... }
}

...

There are some references for implementation:

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

@github-actions github-actions bot added the docs-not-required This change does not impact docs. label May 14, 2024
@zyy17 zyy17 force-pushed the refactor/add-app-builder-trait branch from 03f65e0 to 3b9a4ef Compare May 15, 2024 15:41
@zyy17 zyy17 force-pushed the refactor/add-app-builder-trait branch from 3b9a4ef to d591020 Compare May 18, 2024 03:58
@zyy17
Copy link
Collaborator Author

zyy17 commented May 27, 2024

Close it because it seems the refactoring is unnecessary. I got some new ideas based on the refactoring, but I still need some tests. I will bring the new PR in the future.

@zyy17 zyy17 closed this May 27, 2024
@zyy17 zyy17 deleted the refactor/add-app-builder-trait branch July 2, 2024 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant