> On 2012-02-20 02:38:52, Arvind Prabhakar wrote: > > flume-ng-core/src/main/java/org/apache/flume/SinkProcessor.java, line 28 > > <https://reviews.apache.org/r/3750/diff/5/?file=75935#file75935line28> > > > > 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.
Done this, will be in next patch. I'm not including an abstract processor until we see some other uses and where repetition might be taking place > On 2012-02-20 02:38:52, Arvind Prabhakar wrote: > > flume-ng-core/src/main/java/org/apache/flume/sink/FailoverSinkProcessor.java, > > line 40 > > <https://reviews.apache.org/r/3750/diff/5/?file=75938#file75938line40> > > > > 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. > > > > This is because items without a value are counted down from -1 downwards, leaving postive numbers to be naturally higher priority than unassigned items. Also I like high->high and think there are a lot of places where it is specified that way > On 2012-02-20 02:38:52, Arvind Prabhakar wrote: > > flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java, lines > > 40-45 > > <https://reviews.apache.org/r/3750/diff/5/?file=75939#file75939line40> > > > > <nit> Perhaps this can be folded into Context implementation much like > > context.getSubProperties() does. agreed > On 2012-02-20 02:38:52, Arvind Prabhakar wrote: > > flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java, > > line 322 > > <https://reviews.apache.org/r/3750/diff/5/?file=75943#file75943line322> > > > > 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. This is now a list, and it is up to the processor what to do with it. FailoverProcessor just turns it into a map for lookups > On 2012-02-20 02:38:52, Arvind Prabhakar wrote: > > flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java, > > line 31 > > <https://reviews.apache.org/r/3750/diff/5/?file=75940#file75940line31> > > > > 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. Added in SinkProcessorType and changed the SinkProcessorFactory to use it. - Juhani ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3750/#review5216 ----------------------------------------------------------- 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 > >
