----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/13436/#review24900 -----------------------------------------------------------
Ship it! This looks ready to go. One minor comment below. Also there is a small race condition here. If the flag again is set while the resetConnection is still executing, then resetConnectionFlag is reset. I don't think this is a problem, since the resetConnectionInterval must be >>> time take to reset. It might be a reasonable idea to just add comments mentioning this. Can you please make this change and attach the patch to the jira? flume-ng-core/src/main/java/org/apache/flume/sink/AbstractRpcSink.java <https://reviews.apache.org/r/13436/#comment49063> Nit: This can just be: if(resetConnectionFlag.get()){ .. } - Hari Shreedharan On Aug. 9, 2013, 3:02 a.m., Juhani Connolly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/13436/ > ----------------------------------------------------------- > > (Updated Aug. 9, 2013, 3:02 a.m.) > > > Review request for Flume. > > > Repository: flume-git > > > Description > ------- > > I removed the lock and set a flag in scheduled runnable. This flag is checked > in process and a reconnect is performed if set. > > > Diffs > ----- > > flume-ng-core/src/main/java/org/apache/flume/sink/AbstractRpcSink.java > b3208fc > flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 8760c25 > > Diff: https://reviews.apache.org/r/13436/diff/ > > > Testing > ------- > > Unit tests were adjusted(since a reconnect requires process to actually > happen) and pass. > > We tested with and without the patch on our servers and everything looks good > with log duplication at the same level as having interval = 0 > > > Thanks, > > Juhani Connolly > >
