> 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 > > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line864> > > > > ... if it is asked to do so... 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: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11231/ > ----------------------------------------------------------- > > (Updated June 24, 2013, 8:40 a.m.) > > > Review request for zookeeper. > > > Description > ------- > > ZOOKEEPER-1413: Use on-disk transaction log for learner sync up > > - Use txnlog for learner synchronization if learner fall too far behind > - Refactoring LearnerHandler to deal with different cases of handling learner > synchronization > > > This addresses bug https://issues.apache.org/jira/browse/ZOOKEEPER-1413. > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/ZOOKEEPER-1413 > > > Diffs > ----- > > /ivy.xml 1495522 > /src/java/main/org/apache/zookeeper/server/TxnLogProposalIterator.java > PRE-CREATION > /src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1495522 > /src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java > 1495522 > /src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java > 1495522 > /src/java/main/org/apache/zookeeper/server/persistence/TxnLog.java 1495522 > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java > 1495522 > /src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java > PRE-CREATION > /src/java/test/org/apache/zookeeper/test/FollowerResyncConcurrencyTest.java > 1495522 > /src/java/test/org/apache/zookeeper/test/GetProposalFromTxnTest.java > PRE-CREATION > /src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java 1495522 > > Diff: https://reviews.apache.org/r/11231/diff/ > > > Testing > ------- > > - unit tests > - ran in prod for more than half a year > > > Thanks, > > Thawan Kooburat > >
