Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Zookeeper >> mail # dev >> Review Request: ZOOKEEPER-1413: Use on-disk transaction log for learner sync up


Copy link to this message
-
Re: Review Request: ZOOKEEPER-1413: Use on-disk transaction log for learner sync up

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11231/#review20941
-----------------------------------------------------------
Awesome job, Thawan. I have some comments and questions below, most of them are minor.
/src/java/main/org/apache/zookeeper/server/TxnLogProposalIterator.java
<https://reviews.apache.org/r/11231/#comment43273>

    "... provides an..."

/src/java/main/org/apache/zookeeper/server/TxnLogProposalIterator.java
<https://reviews.apache.org/r/11231/#comment43288>

    Ideally we propagate the exception up, but it is incompatible with the signature of the method unless we throw NoSuchElementException. What if we return null in the case of an exception?

/src/java/main/org/apache/zookeeper/server/ZKDatabase.java
<https://reviews.apache.org/r/11231/#comment43174>

    1/3 seems reasonable but arbitrary. I'm curious to know if there is a concrete reason behind the choice or if it just feels right for the default.

/src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java
<https://reviews.apache.org/r/11231/#comment43310>

    What if we do the fast forwarding in this method instead of having a boolean passed to init?

/src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java
<https://reviews.apache.org/r/11231/#comment43178>

    This fast forward parameter is a bit odd. Need to check why that's necessary and if there is an alternative.

/src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java
<https://reviews.apache.org/r/11231/#comment43315>

    Should we call this method and the one below something like readTxnLog?

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43325>

    I think this means to say that there is no txn for this epoch yet because the leader of the epoch is just starting, is it right?

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43326>

    Make the log info conditional or use curly braces.

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43272>

    typo

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43327>

    I'm not sure I understand the second part of this case. Is this referring to a follower that is starting with a clean state?

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43328>

    "send send"

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43349>

    This comment implies that if peerLastZxid == maxZxid, we still queue maxZxid, but it is not necessary, right? I have a related comment in one of the test cases.

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43346>

    Zab

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43329>

    suggested name for the method: queueCommittedProposals

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43340>

    ... when we see the...

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43335>

    I'm confused about this case. The only way I can think of right now of falling into this case is if the follower has tried to join an epoch that has never become established.
    
    Why does the learner crash?

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43333>

    ... if it is asked to do so...

/src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java
<https://reviews.apache.org/r/11231/#comment43332>

    This seems to be more than a warn, right? It should be either error or fatal.

/src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java
<https://reviews.apache.org/r/11231/#comment43338>

    I'm wondering why we should expect to have a packet in the queue, the peer is already synced. As I understand this is the case in which (peerLaxtZxid, maxZxid] = (1, 1].
- fpj
On May 20, 2013, 5:40 a.m., Thawan Kooburat wrote: