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

Migrate to Slog #185

Merged
merged 8 commits into from
Jun 11, 2019
Merged

Migrate to Slog #185

merged 8 commits into from
Jun 11, 2019

Conversation

nrc
Copy link
Contributor

@nrc nrc commented Feb 20, 2019

Rebased version of #52, based on top of #184

@nrc
Copy link
Contributor Author

nrc commented Feb 21, 2019

rebased

@nrc
Copy link
Contributor Author

nrc commented Feb 22, 2019

Blocked on landing #184

@Hoverbear
Copy link
Contributor

Hoverbear commented Feb 22, 2019

@nrc 2018 is merged now. :)

You have some conflicts due to #127

@nrc
Copy link
Contributor Author

nrc commented Feb 24, 2019

rebased again

@nrc nrc force-pushed the slog branch 2 times, most recently from 90f101a to 361b15c Compare February 25, 2019 03:53
src/lib.rs Outdated Show resolved Hide resolved
harness/src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/progress.rs Outdated Show resolved Hide resolved
src/progress.rs Outdated Show resolved Hide resolved
@Hoverbear Hoverbear added this to the 0.6.0 milestone Feb 25, 2019
@Hoverbear Hoverbear mentioned this pull request Feb 25, 2019
@Hoverbear
Copy link
Contributor

@nrc There is a lot of merge conflicts, do you think you could address them?

@Hoverbear Hoverbear added the Feature Related to a major feature. label Apr 9, 2019
@nrc nrc force-pushed the slog branch 3 times, most recently from 37bfe5d to 82aba5c Compare May 21, 2019 06:25
@nrc
Copy link
Contributor Author

nrc commented May 21, 2019

I've rebased and addressed some of the easier review comments (finally, sorry it took so long). I'll address the other comments tomorrow.

@nrc nrc force-pushed the slog branch 2 times, most recently from a033606 to 501478d Compare May 22, 2019 00:36
@nrc
Copy link
Contributor Author

nrc commented May 22, 2019

All comments addressed. @Hoverbear PTAL

@nrc nrc force-pushed the slog branch 3 times, most recently from 54e70d8 to cb087cb Compare May 30, 2019 22:14
@nrc
Copy link
Contributor Author

nrc commented May 30, 2019

@Hoverbear I changed the API as we discussed and the default logger to be sync. PTAL

Cargo.toml Outdated Show resolved Hide resolved
@nrc
Copy link
Contributor Author

nrc commented Jun 4, 2019

Comment addressed and rebased

Hoverbear and others added 7 commits June 7, 2019 08:06
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Signed-off-by: Nick Cameron <nrc@ncameron.org>
Reduces duplication in log messages, avoids `format!("{:?}", ...)`, and makes logging more idiomatic.

Signed-off-by: Nick Cameron <nrc@ncameron.org>
@nrc
Copy link
Contributor Author

nrc commented Jun 6, 2019

I've improved the logging strings to eliminate some duplication. I doubt things are perfect, but since we need to fix the messages on an individual basis, I think I've hit a reasonable point of diminishing returns

@Hoverbear Hoverbear requested a review from hicqu June 6, 2019 21:33
@Hoverbear
Copy link
Contributor

Whoa check out how nice the output of RUST_TEST_THREADS=1 RUST_LOG=raft=debug cargo test -- --nocapture is!

image

src/raft.rs Outdated Show resolved Hide resolved
msg_index = m.index,
term = self.term;
"tag" => &self.tag,
"msg type" => ?m.msg_type(),
Copy link
Contributor

@Hoverbear Hoverbear Jun 6, 2019

Choose a reason for hiding this comment

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

@breeswish Does this naming style seem ok with you? (spaces in the slog values)

src/raft.rs Outdated Show resolved Hide resolved
src/raft.rs Outdated Show resolved Hide resolved
src/raft.rs Outdated Show resolved Hide resolved
src/raft.rs Outdated Show resolved Hide resolved
src/raft.rs Outdated Show resolved Hide resolved
src/raft_log.rs Outdated Show resolved Hide resolved
src/raft_log.rs Outdated Show resolved Hide resolved
Co-Authored-By: Ana Hobden <operator@hoverbear.org>

Signed-off-by: Nick Cameron <nrc@ncameron.org>
@Hoverbear Hoverbear requested review from breezewish and removed request for hicqu June 10, 2019 19:44
@Hoverbear
Copy link
Contributor

@breeswish PTAL

Copy link
Member

@breezewish breezewish left a comment

Choose a reason for hiding this comment

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

Cool!

@Hoverbear Hoverbear merged commit 4fd768d into tikv:master Jun 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Related to a major feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants