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

Add close method for Region trait #832

Closed
4 tasks
v0y4g3r opened this issue Jan 5, 2023 · 5 comments
Closed
4 tasks

Add close method for Region trait #832

v0y4g3r opened this issue Jan 5, 2023 · 5 comments
Assignees
Labels
C-enhancement Category Enhancements

Comments

@v0y4g3r
Copy link
Contributor

v0y4g3r commented Jan 5, 2023

What type of enhancement is this?

API improvement

What does the enhancement do?

A close method should be added to Region trait so that the resources held by Region can be gracefully release, for example, when a table is dropped.

Implementation challenges

In order to gracefully close a region, we need to implement following tasks:

@evenyag
Copy link
Contributor

evenyag commented Feb 10, 2023

We could acquire the write lock of the region (so we can ensure all pending writes before close are done) and mark the region as closing

@v0y4g3r v0y4g3r assigned v0y4g3r and unassigned v0y4g3r Feb 10, 2023
@WenyXu
Copy link
Member

WenyXu commented Feb 10, 2023

Cool, can I take this

@WenyXu
Copy link
Member

WenyXu commented Feb 14, 2023

I'm currently work on the part that shut down the server gracefully when it received the ctrl-c signal (and I guess we also need to handle the SIGTERM), But I encountered some issues. The Command didn't return a server instance, so I need to design a pattern to notify the instance held by Command.

impl Command {
async fn run(self) -> Result<()> {
self.subcmd.run().await
}
}

The first pattern that came to my mind is to use a channel and pass the channel as a part of ctx into Command's run method. It will look like the following:

  let (rx, tx) = channel();

    let ctx = Context { sigterm: tx };

    tokio::select! {
        result = cmd.run(ctx) => {
            if let Err(err) = result {
                error!(err; "Fatal error occurs!");
            }
        }
        _ = tokio::signal::ctrl_c() => {
            info!("Goodbye!");
            rx.send(());
        }
    }

Then we can handle the signal in the StartCommand's run method.

impl StartCommand {
async fn run(self) -> Result<()> {
logging::info!("Datanode start command: {:#?}", self);
let opts: DatanodeOptions = self.try_into()?;
logging::info!("Datanode options: {:#?}", opts);
Datanode::new(opts)
.await
.context(StartDatanodeSnafu)?
.start()
.await
.context(StartDatanodeSnafu)
}
}

Is this a good solution? Any suggestions are welcomed.

@evenyag
Copy link
Contributor

evenyag commented Feb 15, 2023

Could we add another enum, like Application to wrap structs like Datanode, Frontend?

enum Application {
    Datanode(Datanode),
    Frontend(Frontend),
}

We build the application from the command and run that application.

let app = cmd.build();
tokio::select! {
    result = app.run(ctx) => {},
    _ = tokio::signal::ctrl_c() => {
        app.stop();
    }
}

@evenyag
Copy link
Contributor

evenyag commented Feb 15, 2023

@v0y4g3r Maybe we could change this into a tracking issue to graceful shutdown

@v0y4g3r v0y4g3r closed this as completed Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category Enhancements
Projects
None yet
Development

No branches or pull requests

3 participants