> On Oct. 11, 2012, 11:42 p.m., Mike Percy wrote: > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java, > > line 59 > > <https://reviews.apache.org/r/7550/diff/1/?file=175766#file175766line59> > > > > Why limted private here? What is the implication? > > Brock Noland wrote: > In the InterfaceAudience class, I defined Public to me users of flume. > LimitedPrivate to be only flume developers of other modules "Intended > only for sub-modules of the Flume project" > Private to be within the module itself. > > I think LP or Private makes sense because users of flume shouldn't be > extending FileChannel. > > Honestly we could probably just have Public/Private. > > Mike Percy wrote: > I see. I took it to mean that users should not instantiate FileChannel, > which didn't make much sense to me. > > Brock Noland wrote: > Well they should be instantiating it themselves right? I think in Hadoop > LimitedPrivate and Private apply to end user developers. That is, by > configuring Hadoop you will likely instantiate many Private/LimitedPrivate > objects, but you shouldn't be extending or instantiating in code. If you do, > you know the contract. > > Mike Percy wrote: > In Hadoop I think LimitedPrivate allows you to specify specific other > projects which can use your API. Take Pig's Main class as an example, it > specifies oozie: > http://pig.apache.org/docs/r0.10.0/api/org/apache/pig/Main.html > > Whether they can instantiate FileChannel directly is an interesting > question, because i.e. it may be helpful to do so for unit testing purposes. > Since it implements a public interface Channel then clearly they should be > able to call those methods once they have a Channel implementation instance. > They need some way to instantiate it, maybe DefaultChannelFactory then, if > it's public/stable? > > Brock Noland wrote: > Right, I changed the meaning of LimitedPrivate from other projects to > other modules (in the patch) because I don't see us integrating with out > projects in the same way Hadoop does. I suppose we could keep the meaning and > just not use it for now? > > I would strongly argue that for unit testing purposes they should not be > using FileChannel. They aren't testing FileChannel but their code against the > Channel interface so I think they should use a much simpler Channel for unit > testing like MemoryChannel.
I'd be inclined not to include LimitedPrivate since changing its meaning seems confusing. You make a good argument for not using FileChannel directly, I think adding a few javadoc comments encouraging people to use MemoryChannel for unit testing instead of FC and allowing MemoryChannel to be @Public while making File Channel @Private would clarify matters. These annotations are useful, a little explanation on top likely goes a long way. Anyway I'm not trying to volunteer you for all that work just sharing an observation :) - Mike ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7550/#review12372 ----------------------------------------------------------- On Oct. 11, 2012, 6:24 p.m., Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7550/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2012, 6:24 p.m.) > > > Review request for Flume. > > > Description > ------- > > Add's the Hadoop interface annotations and add's them to a few classes as an > example. Once this approach is agreed upon, any *public* interfaces should be > marked. Private interfaces can be marked in later JIRAs. > > > This addresses bug FLUME-1629. > https://issues.apache.org/jira/browse/FLUME-1629 > > > Diffs > ----- > > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java > 6680a2c > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java > 64725dd > flume-ng-core/src/main/java/org/apache/flume/Channel.java 91ea7b6 > > flume-ng-core/src/main/java/org/apache/flume/annotations/InterfaceAudience.java > PRE-CREATION > > flume-ng-core/src/main/java/org/apache/flume/annotations/InterfaceStability.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/channel/AbstractChannel.java > e662de9 > > Diff: https://reviews.apache.org/r/7550/diff/ > > > Testing > ------- > > > Thanks, > > Brock Noland > >
