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
Thawan Kooburat 2013-06-24, 08:40


> On May 24, 2013, 12:19 a.m., fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/TxnLogProposalIterator.java, line 18
> > <https://reviews.apache.org/r/11231/diff/1/?file=293955#file293955line18>
> >
> >     "... provides an..."

fixed
> On May 24, 2013, 12:19 a.m., fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java, line 523
> > <https://reviews.apache.org/r/11231/diff/1/?file=293957#file293957line523>
> >
> >     What if we do the fast forwarding in this method instead of having a boolean passed to init?
>
> Thawan Kooburat wrote:
>     sure, I can refactor it

fixed
> On May 24, 2013, 12:19 a.m., fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java, line 545
> > <https://reviews.apache.org/r/11231/diff/1/?file=293957#file293957line545>
> >
> >     This fast forward parameter is a bit odd. Need to check why that's necessary and if there is an alternative.
>
> Thawan Kooburat wrote:
>     From learner synchronization perspective, the leader what to send only txns which has zxid > peerLastSeenZxid.  I have to do this either in this layer or in the LearnerHandler. Doing in in this layer is easier to write a unit test and reduce code complexity in LearnerHandler.
>    
>
>
> fpj wrote:
>     Ok, I think my comment is mostly about style, I rarely see an init method with parameters, but there is nothing really broken about doing it.

fixed
> On May 24, 2013, 12:19 a.m., fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java, line 200
> > <https://reviews.apache.org/r/11231/diff/1/?file=293958#file293958line200>
> >
> >     Should we call this method and the one below something like readTxnLog?
>
> Thawan Kooburat wrote:
>     This is an existing method, I think we should only rename a method only if its semantic has changed or the name is really miss leading.
>
> fpj wrote:
>     I think the read method you say exists is in FileTxnLog. My comment is about having a read() method in FileTxnSnapLog because the read could be either for a txn log or a snapshot. I don't think we would be renaming an existing method, but perhaps I'm not getting your comment right.
>
> Thawan Kooburat wrote:
>     Yes, you are right. I was confused with FileTxnLog.read(). Sure, I can rename this one.

fixed
> On May 24, 2013, 12:19 a.m., fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line 699
> > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line699>
> >
> >     Make the log info conditional or use curly braces.
>
> Thawan Kooburat wrote:
>     You mean that I should use this? http://www.slf4j.org/apidocs/org/slf4j/Logger.html#info%28java.lang.String,%20java.lang.Object...%29
>
>
> fpj wrote:
>     Yep!

This requires new version of SLF4J
> On May 24, 2013, 12:19 a.m., fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line 733
> > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line733>
> >
> >     "send send"

fixed
> On May 24, 2013, 12:19 a.m., fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line 812
> > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line812>
> >
> >     Zab

fixed
> On May 24, 2013, 12:19 a.m., fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line 815
> > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line815>
> >
> >     suggested name for the method: queueCommittedProposals
>
> Thawan Kooburat wrote:
>     no problem

fixed
> On May 24, 2013, 12:19 a.m., fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line 841
> > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line841>
> >
> >     ... when we see the...

fixed
> On May 24, 2013, 12:19 a.m., fpj wrote:
> > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line 864

fixed
- Thawan
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/11231/#review20941
On June 24, 2013, 8:40 a.m., Thawan Kooburat wrote: