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

Replaced logging with hclog throughout raft. #360

Merged
merged 3 commits into from
Aug 9, 2019
Merged

Replaced logging with hclog throughout raft. #360

merged 3 commits into from
Aug 9, 2019

Conversation

elliotcourant
Copy link
Contributor

This simply replaces the existing logging implementation with hclog to
make everything consistent throughout the library.

This simply replaces the existing logging implementation with hclog to
make everything consistent throughout the library.
@hashicorp-cla
Copy link

hashicorp-cla commented Aug 4, 2019

CLA assistant check
All committers have signed the CLA.

fuzzy/transport.go Outdated Show resolved Hide resolved
fuzzy/transport.go Outdated Show resolved Hide resolved
net_transport.go Outdated Show resolved Hide resolved
testing.go Outdated Show resolved Hide resolved
@schristoff
Copy link
Contributor

Hey @elliotcourant , thank you SO much for this. Seriously, this is something we've been wanting to have done for awhile. We all really appreciate it (and we all rejoiced when we saw this PR!)

A couple nitpick-y things I commented on above but thought I would expand on. There's some usage of fmt.Sprintf throughout this, I think it may be preferred to use hclog.Fmt() here. It's no big deal either way.

I noted on some comments I saw that may have meant to be deleted, but they also may be necessary, if they are let me know!

Again, thank you 🙏

@elliotcourant
Copy link
Contributor Author

@s-christoff Thank you for the quick follow up!

It would make sense to me to remove those commented out log entries. At this point they could easily be added back in, but since they are not being used they could also be checked with a debugger instead. I will remove them for now.

I'll make the adjustments to use hclog.Fmt(). Should all log entries using fmt be updated as well? (Including ones not touched by this MR?)

Thank you very much!

@elliotcourant
Copy link
Contributor Author

Kind of went nuts and made all of the logging consistent. There are a few lingering fmt.Sprintfs in some tests but in the primary code everything should be consistent with hclog now!

fuzzy/transport.go Outdated Show resolved Hide resolved
@elliotcourant
Copy link
Contributor Author

I had missed several others. Now everything (except for the one test) is using a normal hclog format, or is using hclog.Fmt to format strings.

@s-christoff I hope this is alright, if its not I have not squashed my commits yet so I can revert any changes that you deem unnecessary.

Copy link
Contributor

@schristoff schristoff left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you! This is awesome

Copy link
Contributor

@endocrimes endocrimes left a comment

Choose a reason for hiding this comment

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

LGTM, one question about tests tho

@@ -1856,7 +1856,7 @@ func TestRaft_ProtocolVersion_Upgrade_2_3(t *testing.T) {
// Use the new ID-based API to add the server with its ID.
future := c.Leader().AddVoter(c1.rafts[0].localID, c1.rafts[0].localAddr, 0, 1*time.Second)
if err := future.Error(); err != nil {
c.FailNowf("[ERR] err: %v", err)
c.FailNowf("err: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these test changed accidental? They’re kinda useful for gripping test output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are intentional. The hclog library should include the log level in the entry every time anyway; and c.FailNowf is just a wrapper around an hclog call now. So it will include ERR or ERROR in square brackets for those entries.

@schristoff schristoff closed this Aug 9, 2019
@schristoff schristoff reopened this Aug 9, 2019
@schristoff schristoff merged commit 635796e into hashicorp:master Aug 9, 2019
@schristoff
Copy link
Contributor

🤦‍♀️ Hit the wrong button, sorry

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.

4 participants