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

Miscellaneous godoc and comment improvements #485

Merged
merged 5 commits into from
Jan 24, 2022
Merged

Miscellaneous godoc and comment improvements #485

merged 5 commits into from
Jan 24, 2022

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Jan 13, 2022

While reading the raft source code I found a number of places where the godoc or comments could be improved. I'm happy to revert or make further edits based on review feedback.

I also made a couple super minor code, which I will comment about inline.

These two functions are applying already committed logs to the FSM, so calling them
apply is more consistent.
@dnephin dnephin added the docs label Jan 13, 2022
config.go Outdated Show resolved Hide resolved
api.go Outdated Show resolved Hide resolved
@@ -72,7 +79,7 @@ func (r *Raft) runFSM() {
batchingFSM, batchingEnabled := r.fsm.(BatchingFSM)
configStore, configStoreEnabled := r.fsm.(ConfigurationStore)

commitSingle := func(req *commitTuple) {
applySingle := func(req *commitTuple) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change this function name (and applyBatch below) because the operation it is performing is an Apply, not a commit.

raft.go Outdated Show resolved Hide resolved
Comment on lines -239 to +238
err := BootstrapCluster(&cfg, r.logs, r.stable, r.snapshots,
r.trans, configuration)
err := BootstrapCluster(&cfg, r.logs, r.stable, r.snapshots, r.trans, configuration)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This awkward line wrapping isn't really necessary. Other lines in this file are at least this long, and many are much longer.

api.go Outdated Show resolved Hide resolved
api.go Outdated Show resolved Hide resolved
// current leader is being removed, it will cause a new election
// to occur. This must be run on the leader or it will fail.
// Use RemoveServer instead.
// RemovePeer from the cluster configuration. If the current leader is being
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a word in first sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find many examples of this style, but I think they exist somewhere. Maybe I'll stumble on them again sometime. This style uses the method name as part of the sentence, so "remove peer from the cluster configuration".

raft.go Outdated Show resolved Hide resolved
Co-authored-by: Nick Cabatoff <ncabatoff@hashicorp.com>
@dnephin dnephin merged commit 09ad89e into main Jan 24, 2022
@dnephin dnephin deleted the dnephin/godoc-1 branch January 24, 2022 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants