Require documentation on public interfaces #87

choubacha · 2018-07-09T22:10:36Z

Previously, compiling would not warn or error for lack of documentation.
Now the compiler will fail if public interfaces are left without
documentation. To prevent the library from failing, all public
interfaces have been documented.

resolves #72

Critique welcome: I tried to document the functions as I understood them but there
may be places where I have typos or misunderstood.

I also noticed that some of these functions should probably be scoped to the crate instead of
making them public. I did not change them, however, as that would potentially break the interface.
Could you let me know if any of these functions can be converted to pub(crate)?

Hoverbear · 2018-07-09T22:24:59Z

src/lib.rs

@@ -224,6 +225,8 @@ extern crate protobuf;
 extern crate quick_error;
 extern crate rand;

+/// This module supplies the needed message types. However, it is autogenerated and thus cannot be


I think we could examine having the generated code use comments from the proto files as well. 🤔

That would be really cool!

Hoverbear · 2018-07-09T22:26:54Z

src/log_unstable.rs

@@ -148,6 +166,8 @@ impl Unstable {
        &self.entries[l - off..h - off]
    }

+    /// Asserts the hi and lo values against each other and against the


Can you throw hi and lo in ticks so they're formatted distinctly?

Hoverbear · 2018-07-09T22:32:15Z

src/progress.rs

    pub fn iter_mut(&mut self) -> Chain<IterMut<u64, Progress>, IterMut<u64, Progress>> {
        self.voters.iter_mut().chain(&mut self.learners)
    }

+    /// Adds a voter node
+    ///
+    /// # Panics


This can be avoided. :) Thanks: #88

Hoverbear · 2018-07-09T22:36:29Z

src/progress.rs

    pub fn remove(&mut self, id: u64) -> Option<Progress> {
        match self.voters.remove(&id) {
            None => self.learners.remove(&id),
            some => some,
        }
    }

+    /// Promote a learner to a peer.
+    ///
+    /// # Panics


Hoverbear · 2018-07-09T22:40:47Z

src/raft.rs

    Leader,
+    /// The node could become a candidate.


Please add a note that this only happens if prevote is enabled.

Hoverbear · 2018-07-09T22:42:39Z

src/raft.rs

@@ -753,6 +794,8 @@ impl<T: Storage> Raft<T> {
        }
    }

+    /// Appends an slice of entries to the log. The entries are updated to match


Appends a slice

Hoverbear · 2018-07-09T22:53:17Z

src/raw_node.rs

-// All fields in Ready are read-only.
+/// Ready encapsulates the entries and messages that are ready to read,
+/// be saved to stable storage, committed or sent to other peers.
+/// All fields in Ready are read-only.


Hoverbear · 2018-07-09T22:55:24Z

src/status.rs

    pub hs: HardState,
+    /// The softstate of the raft, representing prosposed state.


Hoverbear

I notice that there is consistency issues between some comments (no capital at the start.) and the new ones (Capital at the start.).

Would you be willing to fix them to all be one style at least for this diff? (`Capital at the front works for me.) If not I can follow up.

siddontang · 2018-07-11T01:27:11Z

PTAL @queenypingcap

choubacha · 2018-07-11T03:03:52Z

@Hoverbear PTAL

CaitinChen · 2018-07-11T03:21:01Z

proto/eraftpb.proto

@@ -6,6 +6,16 @@ enum EntryType {
    EntryConfChange = 1;
 }

+// The entry is a type of change that needs to be applied. It contains two data fields.
+// Which field is used and how can be determined by the entry type.


-> Which field is used and how it can be determined by the entry type.

I think it should be:

Which field is used, and how, is determined by the entry type.

The usage and how it's used is determined by the type. I'm rewording it completely though to see if I can make it more obvious.

While the fields are built into the model; their usage is determined by the entry_type.

CaitinChen · 2018-07-11T03:35:17Z

src/progress.rs

+    /// an optimized state for fast replicating log entries to the follower.
+    ///
+    /// When in ProgressStateSnapshot, leader should have sent out snapshot
+    /// before and stops sending any replication message.


stops -> stop

CaitinChen · 2018-07-11T03:38:56Z

src/progress.rs

@@ -200,28 +233,31 @@ impl Progress {
        }
    }

+    /// Changes the progress to a a Replicate.


Changes the progress to a Replicate.

CaitinChen · 2018-07-11T03:41:54Z

src/progress.rs

    pub fn optimistic_update(&mut self, n: u64) {
        self.next_idx = n + 1;
    }

-    // maybe_decr_to returns false if the given to index comes from an out of order message.
-    // Otherwise it decreases the progress next index to min(rejected, last) and returns true.
+    /// Returns false if the given to index comes from an out of order message.


-> Returns false if the given index comes from an out of order message. ?

CaitinChen · 2018-07-11T03:50:41Z

src/raft.rs

@@ -444,38 +472,45 @@ impl<T: Storage> Raft<T> {
        r
    }

+    /// Grabs a immutable reference to the store.


CaitinChen · 2018-07-11T06:55:18Z

src/raft.rs

@@ -2060,6 +2132,7 @@ impl<T: Storage> Raft<T> {
        self.election_elapsed >= self.randomized_election_timeout
    }

+    /// Regenerates and stroes the election timeout.


stroes -> stores

CaitinChen · 2018-07-11T07:02:36Z

src/raft_log.rs

+    /// It returns the first index of conflicting entries between the existing
+    /// entries and the given entries, if there are any.
+    ///
+    /// If there is no conflicting entries, and the existing entries contain


-> If there are no conflicting entries, and the existing entries contain...

CaitinChen · 2018-07-11T07:03:13Z

src/raft_log.rs

+    /// If there is no conflicting entries, and the existing entries contain
+    /// all the given entries, zero will be returned.
+    ///
+    /// If there is no conflicting entries, but the given entries contains new


-> If there are no conflicting entries, but the given entries contain new...

CaitinChen · 2018-07-11T07:26:35Z

src/raw_node.rs

-    // Returns true to indicate that there will probably be some readiness need to be handled.
+    /// Tick advances the internal logical clock by a single tick.
+    ///
+    /// Returns true to indicate that there will probably be some readiness need to be handled.


Returns true to indicate that there will probably be some readiness which need to be handled.

CaitinChen · 2018-07-11T07:36:35Z

src/raw_node.rs

+    /// ReadIndex requests a read state. The read state will be set in ready.
+    /// Read State has a read index. Once the application advances further than the read
+    /// index, any linearizable read requests issued before the read request can be
+    /// processed safely. The read state will have the same rctx attched.


attched -> attached

choubacha · 2018-07-11T15:39:51Z

@CaitinChen Thank you for the thorough review! PTAL

Previously, compiling would not warn or error for lack of documentation. Now the compiler will fail if public interfaces are left without documentation. To prevent the library from failing, all public interfaces have been documented. resolves tikv#72

Hoverbear

Thank you for this valuable contribution. 🙇‍♀️ :

CaitinChen

LGTM

choubacha force-pushed the choubacha/deny-missing-docs branch from 622325c to 3dea091 Compare July 9, 2018 22:15

Hoverbear reviewed Jul 9, 2018

View reviewed changes

src/status.rs Outdated

pub hs: HardState,

/// The softstate of the raft, representing prosposed state.

Copy link

Contributor

Hoverbear Jul 9, 2018

Choose a reason for hiding this comment

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

proposed

Hoverbear reviewed Jul 9, 2018

View reviewed changes

choubacha force-pushed the choubacha/deny-missing-docs branch from 3dea091 to 6804476 Compare July 11, 2018 03:03

choubacha force-pushed the choubacha/deny-missing-docs branch from 6804476 to a20eff4 Compare July 11, 2018 03:06

Hoverbear added this to the 0.4.0 milestone Jul 11, 2018

Hoverbear added the Enhancement An improvement to existing code. label Jul 11, 2018

CaitinChen reviewed Jul 11, 2018

View reviewed changes

choubacha force-pushed the choubacha/deny-missing-docs branch from a20eff4 to f26ba85 Compare July 11, 2018 15:39

choubacha force-pushed the choubacha/deny-missing-docs branch from f26ba85 to 8772e69 Compare July 11, 2018 15:41

Hoverbear approved these changes Jul 11, 2018

View reviewed changes

CaitinChen approved these changes Jul 13, 2018

View reviewed changes

Merge branch 'master' into choubacha/deny-missing-docs

c000202

Hoverbear merged commit 18f1cd8 into tikv:master Jul 13, 2018

Hoverbear mentioned this pull request Jul 13, 2018

config: Extract to own file. #91

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require documentation on public interfaces #87

Require documentation on public interfaces #87

choubacha commented Jul 9, 2018

Hoverbear Jul 9, 2018

choubacha Jul 11, 2018

Hoverbear Jul 9, 2018

Hoverbear Jul 9, 2018

Hoverbear Jul 9, 2018

Hoverbear Jul 9, 2018

Hoverbear Jul 9, 2018

Hoverbear Jul 9, 2018

Hoverbear Jul 9, 2018

Hoverbear left a comment

siddontang commented Jul 11, 2018

choubacha commented Jul 11, 2018

CaitinChen Jul 11, 2018

choubacha Jul 11, 2018

CaitinChen Jul 11, 2018

CaitinChen Jul 11, 2018

CaitinChen Jul 11, 2018

CaitinChen Jul 11, 2018

CaitinChen Jul 11, 2018

choubacha Jul 11, 2018

CaitinChen Jul 11, 2018

CaitinChen Jul 11, 2018

CaitinChen Jul 11, 2018

CaitinChen Jul 11, 2018

choubacha commented Jul 11, 2018

Hoverbear left a comment

CaitinChen left a comment

		pub hs: HardState,
		/// The softstate of the raft, representing prosposed state.

Require documentation on public interfaces #87

Require documentation on public interfaces #87

Conversation

choubacha commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoverbear left a comment

Choose a reason for hiding this comment

siddontang commented Jul 11, 2018

choubacha commented Jul 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

choubacha commented Jul 11, 2018

Hoverbear left a comment

Choose a reason for hiding this comment

CaitinChen left a comment

Choose a reason for hiding this comment