> On Dec. 9, 2012, 12:24 a.m., Mike Percy wrote: > > Brock, looks very nice overall. > > > > Some general suggestions: > > 1. Can we try to add a Javadoc at the top of every interface and class? It > > helps to specifies its purpose for other devs / reviewers. > > 2. Does the EmbeddedAgentConfiguration support making the channel > > "optional" via a channel selector? I can't tell. If not, can we add support > > for that? > > 3. Needs a doc section in the Flume developer guide > > > > Nit: a few whitespace red things on RB > > > > Other specific feedback inline. > > > > Brock Noland wrote: > Hi Mike, > > Thank you for the feedback! Regarding 1, 3, and the whitespace I agree > whole heartedly. As I said in in the RB description these are items I plan on > addressing in the next iteration. This iteration was to solicit feedback on > the approach. Regarding #2, in the design document we specified only a single > channel would be allowed. If we allowed that channel to be optional I'd think > the user might as well use the RPCClient. What do you think? > > I'll address your feedback in the next iteration of the patch! Thanks > again! > > Brock > > Brock Noland wrote: > Comments address in the latest patch which will be uploaded in a few > minutes.
Thanks Brock! The use case for #2 is that a user of the embedded agent may want to drop events when the channel is full. However, now that I've thought about it some more, they could simply log the resulting EventDeliveryException or ChannelException and continue on without retrying. So unless you can think of a reason why that behavior wouldn't sufficiently satisfy such a use case, this should be fine. - Mike ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8127/#review14085 ----------------------------------------------------------- On Dec. 10, 2012, 8:36 p.m., Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8127/ > ----------------------------------------------------------- > > (Updated Dec. 10, 2012, 8:36 p.m.) > > > Review request for Flume. > > > Description > ------- > > Patch is on top of the FLUME-1502 branch which has 1630 committed to it. > > This patch creates a module flume-ng-embedded-agent which implements > FLUME-1502. The patch is not read for commit. Specifically, it needs the > following: > > 1) whitespace removal > 2) docs in package-info.java and the developer guide > > However, I'd like to start soliciting feedback as these are not items which > will affect it's overall design. > > > This addresses bug FLUME-1502. > https://issues.apache.org/jira/browse/FLUME-1502 > > > Diffs > ----- > > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java > 5d56599 > flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java > f39b919 > flume-ng-dist/pom.xml d929c88 > flume-ng-doc/sphinx/FlumeDeveloperGuide.rst f52970b > flume-ng-embedded-agent/pom.xml PRE-CREATION > > flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/EmbeddedAgent.java > PRE-CREATION > > flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/EmbeddedAgentConfiguration.java > PRE-CREATION > > flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/EmbeddedSource.java > PRE-CREATION > > flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/MaterializedConfigurationProvider.java > PRE-CREATION > > flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/MemoryConfigurationProvider.java > PRE-CREATION > > flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/package-info.java > PRE-CREATION > > flume-ng-embedded-agent/src/test/java/org/apache/flume/agent/embedded/TestEmbeddedAgent.java > PRE-CREATION > > flume-ng-embedded-agent/src/test/java/org/apache/flume/agent/embedded/TestEmbeddedAgentConfiguration.java > PRE-CREATION > > flume-ng-embedded-agent/src/test/java/org/apache/flume/agent/embedded/TestEmbeddedAgentEmbeddedSource.java > PRE-CREATION > > flume-ng-embedded-agent/src/test/java/org/apache/flume/agent/embedded/TestEmbeddedAgentState.java > PRE-CREATION > flume-ng-embedded-agent/src/test/resources/log4j.properties PRE-CREATION > > flume-ng-node/src/main/java/org/apache/flume/node/MaterializedConfiguration.java > 674fb38 > pom.xml 53ac96b > > Diff: https://reviews.apache.org/r/8127/diff/ > > > Testing > ------- > > Additional unit tests have been added and they pass. > > > Thanks, > > Brock Noland > >
