> 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. > >
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. > 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. 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. > On May 24, 2013, 12:19 a.m., fpj wrote: > > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line > > 677 > > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line677> > > > > 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? > > Thawan Kooburat wrote: > Any zxid should point to a valid txn. However, this zxid point to > NEWLEADER txn which is never persist to txnlog. If I don't have special > handling for this case, the leader will think that this learner has a txn > which it has never seen. So it will send TRUNC message to the learner and > cause it to crash. > > Some follower may report that it has this zxid, if it gets a snapshot > when the quorum is recently formed or get NEWLEADER package as part of > outstanding proposals. > > You may also noticed that this patch only changes the leader side. This > is intentional because I need to support rolling upgrade. Ok, sounds good. > 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 > > Yep! > On May 24, 2013, 12:19 a.m., fpj wrote: > > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line > > 726 > > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line726> > > > > 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? > > Thawan Kooburat wrote: > See my comment about NEWLEADER zxid above Ok. > On May 24, 2013, 12:19 a.m., fpj wrote: > > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line > > 806 > > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line806> > > > > 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. > > Thawan Kooburat wrote: > If there is a better math notation, let me know. This method won't queue > maxZxid in this case. No, I think this is ok, I just wanted to make sure I had the right understanding. > On May 24, 2013, 12:19 a.m., fpj wrote: > > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line > > 861 > > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line861> > > > > 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? > > Thawan Kooburat wrote: > This is also a case when quorum is formed and it has never see any > request. Then, we restart the quorum. When you say that a quorum is formed, which point of the protocol you're referring to? When a quorum has acknowledged NEWLEADER or some earlier point? > On May 24, 2013, 12:19 a.m., fpj wrote: > > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java, line > > 866 > > <https://reviews.apache.org/r/11231/diff/1/?file=293960#file293960line866> > > > > This seems to be more than a warn, right? It should be either error or > > fatal. > > Thawan Kooburat wrote: > Should be WARN or INFO because we just send SNAP whenever we have a case > that we cannot handle. It is correct but not optimized. You're right, but the logic doesn't seem completely straightforward to me. Is it the case that we end up sending a snapshot because of the special case handled by this if block? if (needOpPacket && !needSnap) { // This should never happen, but we should fall back to sending // snapshot just in case. LOG.error("Unhandled scenario for peer sid: " + getSid() + " fall back to use snapshot"); needSnap = true; } If this is right, why do we need special handling instead of just capturing the fact we need a snapshot from the return value of queuedCommittedProposal? > On May 24, 2013, 12:19 a.m., fpj wrote: > > /src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java, > > line 216 > > <https://reviews.apache.org/r/11231/diff/1/?file=293961#file293961line216> > > > > 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]. > > Thawan Kooburat wrote: > DIFF packet Ok. > On May 24, 2013, 12:19 a.m., fpj wrote: > > /src/java/main/org/apache/zookeeper/server/TxnLogProposalIterator.java, > > line 67 > > <https://reviews.apache.org/r/11231/diff/1/?file=293955#file293955line67> > > > > 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? > > Thawan Kooburat wrote: > IOException can be thrown from > > hasNext = itr.next(); > > which for prefetching the next element. > > I think we only need to set hasNext to false when we see exception, so > the client won't try to call next() again > > > Raul Gutierrez Segales wrote: > Record#serialize() can also throw IOException. Are callers of this method > expected to deal with a Proposal that isn't properly initialized (i.e.: won't > have the QuorumPacket member won't be initialized so might have wrong zxid, > type, etc)? > > Thawan Kooburat wrote: > itr.next() is the only place that actually perform IO-related operation. > > Record#serialize() can thrown IO exception if the output stream is > writing to network or disk. However, in this case, we are writing it to > memory because we are using BinaryOutputArchive/ByteArrayOutputStream. I > looked at the code, these implementation cannot throw IOException but they > have to inherit the method signature of output stream. My concern here is that we simply log the error and keep making progress regularly, that's why I suggested we could try to propagate an exception up, but it is not completely straightforward because of the reasons I stated. If a server acks a txn but later cannot read it, then we might end up in an inconsistent state. - fpj ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/11231/#review20941 ----------------------------------------------------------- On May 20, 2013, 5:40 a.m., Thawan Kooburat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/11231/ > ----------------------------------------------------------- > > (Updated May 20, 2013, 5: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 > ----- > > /src/java/main/org/apache/zookeeper/server/TxnLogProposalIterator.java > PRE-CREATION > /src/java/main/org/apache/zookeeper/server/ZKDatabase.java 1483440 > /src/java/main/org/apache/zookeeper/server/persistence/FileTxnLog.java > 1483440 > /src/java/main/org/apache/zookeeper/server/persistence/FileTxnSnapLog.java > 1483440 > /src/java/main/org/apache/zookeeper/server/persistence/TxnLog.java 1483440 > /src/java/main/org/apache/zookeeper/server/quorum/LearnerHandler.java > 1483440 > /src/java/test/org/apache/zookeeper/server/quorum/LearnerHandlerTest.java > PRE-CREATION > /src/java/test/org/apache/zookeeper/test/FollowerResyncConcurrencyTest.java > 1483440 > /src/java/test/org/apache/zookeeper/test/GetProposalFromTxnTest.java > PRE-CREATION > /src/java/test/org/apache/zookeeper/test/LoadFromLogTest.java 1483440 > > Diff: https://reviews.apache.org/r/11231/diff/ > > > Testing > ------- > > - unit tests > - ran in prod for more than half a year > > > Thanks, > > Thawan Kooburat > >
