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

*: flush log before exit #3594

Merged
merged 8 commits into from Sep 14, 2018
Merged

*: flush log before exit #3594

merged 8 commits into from Sep 14, 2018

Conversation

overvenus
Copy link
Member

Signed-off-by: Neil Shen overvenus@gmail.com

What have you changed? (mandatory)

Flush remaining logs before tikv exits.

What are the type of the changes? (mandatory)

  • Bug fix (non-breaking change which fixes an issue)

How has this PR been tested? (mandatory)

Unit tests.

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

No.

Does this PR affect tidb-ansible update? (mandatory)

No.

Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
@@ -79,6 +80,12 @@ pub fn set_exit_hook(panic_abort: bool) {
.spawn(Backtrace::new)
.unwrap();

// Hold the guard.
lazy_static! {
pub static ref LOG_GUARD: Mutex<Option<GlobalLoggerGuard>> = Mutex::new(None);
Copy link
Member

Choose a reason for hiding this comment

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

Why make it static?

Copy link
Member Author

Choose a reason for hiding this comment

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

To work around cannot move out of captured outer variable in an Fn closure.

Copy link
Member

Choose a reason for hiding this comment

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

It's Mutex instead of static that fixes the error. 😆

@@ -105,6 +112,10 @@ pub fn set_exit_hook(panic_abort: bool) {
} else {
orig_hook(info);
}

// To collect remaining logs, drop guard before exit.
drop(LOG_GUARD.lock().unwrap().take());
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem like flushing the log. What if there is a thread that keeps writing logs?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the Aync Drainer, drop the guard will send a AsyncMsg::Finish which will stop the logger, it accepts no more logs, so drop returns eventually.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds hacky. Is there any more verbose and straightforward way?

Signed-off-by: Neil Shen <overvenus@gmail.com>
Signed-off-by: Neil Shen <overvenus@gmail.com>
@@ -63,7 +64,7 @@ fn track_hook(p: &PanicInfo) {
}

/// Exit the whole process when panic.
pub fn set_exit_hook(panic_abort: bool) {
pub fn set_exit_hook(panic_abort: bool, guard: Option<GlobalLoggerGuard>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to define an OnHook trait for more customization, not only for Logger here.

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 think it is enough, there are few things need to be called before panicking. And we can add the trait when there are more things.

@@ -36,6 +36,7 @@ extern crate slog_async;
extern crate slog_scope;
extern crate slog_stdlog;
extern crate slog_term;
#[macro_use]
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

  --> src/bin/util/setup.rs:62:26
   |
62 |             .thread_name(thd_name!("term-slogger"))

Copy link
Member

@BusyJay BusyJay left a comment

Choose a reason for hiding this comment

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

LGTM, maybe setup.rs can be moved to library source.

@siddontang
Copy link
Contributor

PTAL @Hoverbear

Copy link
Contributor

@Hoverbear Hoverbear left a comment

Choose a reason for hiding this comment

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

Nice catch. :)

@overvenus
Copy link
Member Author

/run-all-tests

@overvenus overvenus merged commit 90d21a0 into master Sep 14, 2018
@overvenus overvenus deleted the flush-logs branch September 14, 2018 09:06
sticnarf pushed a commit to sticnarf/tikv that referenced this pull request Oct 27, 2019
Signed-off-by: Neil Shen <overvenus@gmail.com>
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.

None yet

4 participants