----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3750/#review5216 -----------------------------------------------------------
Thanks for the patch Juhani. The change is good for checkin. I have some feedback below, feel free to push back and create follow-up issues to address those if you would so prefer. * Javadocs for new interfaces and default implementation would be great. * Since the javadoc on PropertiesFileConfigurationProvider are comprehensive, a brief mention of the group semantics in that would help keep it up-to-date. * In a few places the lines are over 80 character limit. I typically set the "show print margine" on my eclipse workbench and it helps identify those violations. Some more feedback follows: flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java <https://reviews.apache.org/r/3750/#comment11411> Since the sinks are named components anyway, the passing of Map<> keyed on the names is not necessary. Using a Map here makes ambiguous the order of declaration which may be of some consequence to the sink processor. Hence I suggest using a List instead. We can have an AbstractSinkProcessor which implements this method and internally constructs a map for quick lookups by the child classes. flume-ng-core/src/main/java/org/apache/flume/sink/FailoverSinkProcessor.java <https://reviews.apache.org/r/3750/#comment11410> In the current implementation it seems that the priority of the sink is proportional to the priority value specified. Usually its the other way around - where lower priority value means higher priority. flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java <https://reviews.apache.org/r/3750/#comment11412> <nit> Perhaps this can be folded into Context implementation much like context.getSubProperties() does. flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java <https://reviews.apache.org/r/3750/#comment11413> Following the convention for other named component types - it will be better to have an Enum for SinkProcessorType which defines the short name and implementation class name. flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java <https://reviews.apache.org/r/3750/#comment11408> Suggest you use a LinkedHashMap here to preserve the order of declaration of sinks. Otherwise, this order of declaration will be lost when SinkGroup is instantiated. While not every group/processor would need to know the order, we should preserve it nevertheless. - Arvind On 2012-02-20 01:46:41, Juhani Connolly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/3750/ > ----------------------------------------------------------- > > (Updated 2012-02-20 01:46:41) > > > Review request for Flume. > > > Summary > ------- > > This includes the changes from FLUME-945 and is thus subject to change with > it. I have a separate patch for the diff from 945->865 if someone wants it. > > A failover sink runner is added, along with a SinkRunnerFactory. The > configuration reading is modified so as to read the runner configuration and > pass it to the SinkRunnerFactory. > A failover sink is used by setting the runner type to failover for every > participating sink. In addition each sink assigned to a runner must set the > same runner.name and a unique runner.priority > The runner will poll only the highest priority live sink. Should it fail by > throwing EventDeliveryException, it will throw it into a pile of dead sinks. > Only once all sinks are exhausted are the dead sinks revived. > > Some issues that are outstanding > - Not all sinks throw EventDeliveryException, or have a clear time when they > should be considered "dead". Coupling such sinks with the failover runner > will not result in failover > - Sinks do not have a clear mechanism to test for their liveliness, other > than perhaps polling LifecycleState. However for most sinks lifecyclestate > tends to remain in START even if the sink is unable to do anything(e.g. avro > sink failing to connect) > > > This addresses bug FLUME-865. > https://issues.apache.org/jira/browse/FLUME-865 > > > Diffs > ----- > > flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/SinkRunner.java 074aab3 > flume-ng-core/src/main/java/org/apache/flume/sink/DefaultSinkProcessor.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/sink/FailoverSinkProcessor.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java > PRE-CREATION > > flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java > PRE-CREATION > > flume-ng-node/src/main/java/org/apache/flume/conf/properties/FlumeConfiguration.java > 1ee1f8e > > flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java > 3d0e366 > > flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java > 34d6010 > flume-ng-node/src/test/resources/flume-conf.properties eebf03d > > Diff: https://reviews.apache.org/r/3750/diff > > > Testing > ------- > > All unit tests pass. > A new unit test was added. It creates a memory channel and a failover runner > with 3 sinks that consume a preset number of events then send > EventDeliveryException. Events are fed to the channel and assertions are made > that the runner is failing over to the correct sinks. This test also passes. > > I also tried to run this on a real cluster, with one master source using a > failover runner to three avro sinks which would each fed into separate agents > which I would kill off to test the failover. Unfortunately because of > AvroSink not throwing EventDeliveryException, this test could not be > completed succesfully, and I felt it was beyond the scope of this ticket to > modify AvroSink > > > Thanks, > > Juhani > >
