----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7518/#review13760 -----------------------------------------------------------
Looks good. I have some minor comments, many are really nits. Getting the channel's class object is delegated to getChannelClass method - but this isn't done in the DefaultSinkFactory or DefaultSourceFactory - making it uniform makes sense right? flume-ng-core/src/main/java/org/apache/flume/channel/DefaultChannelFactory.java <https://reviews.apache.org/r/7518/#comment29431> Looks like an outdated comment? flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java <https://reviews.apache.org/r/7518/#comment29449> Nit: A HashmultiMap would be faster I think, but given that this is not going to be more than tens of elements at most, I don't think it matters. flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java <https://reviews.apache.org/r/7518/#comment29452> Nit: This map is cleared at the end of this method after its use, it does not need to be cleared now. flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java <https://reviews.apache.org/r/7518/#comment29450> Just to be clear: This is meant for reconfiguration right? To take care of old config's channels which will no longer be used? flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java <https://reviews.apache.org/r/7518/#comment29458> It is not immediately evident how the "used channels" are removed from the unused channels map. I'd recommend adding some comments here to explain how it happens. This can be moved to the loadChannels method I think flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java <https://reviews.apache.org/r/7518/#comment29451> This line can be moved out of the inner loop, since this we need to retrieve the channelMap only once per channel class, so this could become: <outer-loop> channelMap = channels.get(channelKlass) if(channelMap!= null) { <inner-loop> } flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java <https://reviews.apache.org/r/7518/#comment29457> Could you add some javadocs to this method and also make it clear that this method removes the channels from the unusedChannels method? It is not evident from the code in getConfiguration() flume-ng-node/src/main/java/org/apache/flume/node/Application.java <https://reviews.apache.org/r/7518/#comment29460> This should also stop the monitorServer flume-ng-node/src/main/java/org/apache/flume/node/Application.java <https://reviews.apache.org/r/7518/#comment29459> Can't we get rid of either the start or the startAllComponents methods? Both do the same thing, we should essentially have just one. If reload is true, register with the eventBus else don't - the logic should exactly the same right? flume-ng-node/src/main/java/org/apache/flume/node/MaterializedConfiguration.java <https://reviews.apache.org/r/7518/#comment29464> I'd prefer adding methods to add the channels, sources and sinks, rather than exposing the maps and have other classes add components to that map. flume-ng-node/src/main/java/org/apache/flume/node/PollingPropertiesFileConfigurationProvider.java <https://reviews.apache.org/r/7518/#comment29462> We can replace this with: while(!executorService.awaitTermination(500, TimeUnit.MILLISECONDS)) { <log> } flume-ng-node/src/main/java/org/apache/flume/node/PollingPropertiesFileConfigurationProvider.java <https://reviews.apache.org/r/7518/#comment29463> We should probably not loop continously. We should just sleep for 30 seconds in between flume-ng-node/src/main/java/org/apache/flume/node/PropertiesFileConfigurationProvider.java <https://reviews.apache.org/r/7518/#comment29461> Is this an old comment? - Hari Shreedharan On Oct. 11, 2012, 6:58 p.m., Brock Noland wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7518/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2012, 6:58 p.m.) > > > Review request for Flume. > > > Description > ------- > > Patch is not for commit, yet. It changes the configuration system into > something extendable and maintainable. Additionally it changes the > terminology from node to agent. Once the patch is ready for review we should > change the node package to agent to conform to the agent terminology. > > Big ticket items: > > 1) Abstract property file provider is changed to Abstract property provider. > Two concrete implementations are provided, PropertyFileConfigurationProvider > and PollingPropertyFileConfigurationProvider. There is an additional concrete > implementation MemoryConfigurationProvider is in > TestAbstractConfigurationProvider. > > 2) Caching instances is removed from all factories. Instance caching is > implemented in AbstractConfigurationProvider for channels *if* they have the > Reusable annotation. MemoryChannel has this annotation. > > 3) A layer of supervisors is removed. The application class now starts and > stops the components when handleConfigurationEvent is called. This is called > on startup if PropertyFileConfigurationProvider is used or whenever the > configuration file changes if PollingPropertyFileConfigurationProvider is > used. PollingPropertyFileConfigurationProvider uses EventBus (guava) to > trigger the re-configuration. > > > This addresses bug FLUME-1630. > https://issues.apache.org/jira/browse/FLUME-1630 > > > Diffs > ----- > > > flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/FileChannel.java > 6680a2c > > flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/JdbcChannel.java > bca0c50 > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/RecoverableMemoryChannel.java > 49e7cfd > > flume-ng-configuration/src/main/java/org/apache/flume/conf/FlumeConfiguration.java > 9b209e8 > flume-ng-core/src/main/java/org/apache/flume/ChannelFactory.java e269909 > flume-ng-core/src/main/java/org/apache/flume/Constants.java 4c6992d > flume-ng-core/src/main/java/org/apache/flume/SinkFactory.java d8fd0da > flume-ng-core/src/main/java/org/apache/flume/SourceFactory.java 91cc866 > > flume-ng-core/src/main/java/org/apache/flume/channel/DefaultChannelFactory.java > ee32696 > flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java > fc3a1e2 > flume-ng-core/src/main/java/org/apache/flume/sink/DefaultSinkFactory.java > e71d44e > > flume-ng-core/src/main/java/org/apache/flume/source/DefaultSourceFactory.java > 18533ae > > flume-ng-core/src/test/java/org/apache/flume/sink/TestDefaultSinkFactory.java > ab3f447 > > flume-ng-core/src/test/java/org/apache/flume/source/TestDefaultSourceFactory.java > ff5b4d6 > flume-ng-node/pom.xml 5464bd3 > > flume-ng-node/src/main/java/org/apache/flume/conf/file/AbstractFileConfigurationProvider.java > a2c882b > > flume-ng-node/src/main/java/org/apache/flume/conf/file/SimpleNodeConfiguration.java > 99b8bcc > > flume-ng-node/src/main/java/org/apache/flume/conf/properties/PropertiesFileConfigurationProvider.java > 8dbbe57 > > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java > PRE-CREATION > flume-ng-node/src/main/java/org/apache/flume/node/Application.java 405afa3 > > flume-ng-node/src/main/java/org/apache/flume/node/ConfigurationProvider.java > ae732aa > flume-ng-node/src/main/java/org/apache/flume/node/FlumeNode.java cee022d > > flume-ng-node/src/main/java/org/apache/flume/node/MaterializedConfiguration.java > PRE-CREATION > flume-ng-node/src/main/java/org/apache/flume/node/NodeConfiguration.java > a24c939 > flume-ng-node/src/main/java/org/apache/flume/node/NodeManager.java 7457d87 > > flume-ng-node/src/main/java/org/apache/flume/node/PollingPropertiesFileConfigurationProvider.java > PRE-CREATION > > flume-ng-node/src/main/java/org/apache/flume/node/PropertiesFileConfigurationProvider.java > PRE-CREATION > > flume-ng-node/src/main/java/org/apache/flume/node/SimpleMaterializedConfiguration.java > PRE-CREATION > > flume-ng-node/src/main/java/org/apache/flume/node/nodemanager/AbstractLogicalNodeManager.java > 1fda07b > > flume-ng-node/src/main/java/org/apache/flume/node/nodemanager/DefaultLogicalNodeManager.java > 60bfd5e > > flume-ng-node/src/main/java/org/apache/flume/node/nodemanager/NodeConfigurationAware.java > c20bf9b > > flume-ng-node/src/test/java/org/apache/flume/conf/properties/TestPropertiesFileConfigurationProvider.java > d43aed6 > > flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java > PRE-CREATION > > flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractLogicalNodeManager.java > 1cbc269 > flume-ng-node/src/test/java/org/apache/flume/node/TestApplication.java > PRE-CREATION > > flume-ng-node/src/test/java/org/apache/flume/node/TestDefaultLogicalNodeManager.java > 530b299 > flume-ng-node/src/test/java/org/apache/flume/node/TestFlumeNode.java > f2dad6f > > flume-ng-node/src/test/java/org/apache/flume/node/TestFlumeNodeApplication.java > f759af1 > > flume-ng-node/src/test/java/org/apache/flume/node/TestPollingPropertiesFileConfigurationProvider.java > PRE-CREATION > > flume-ng-node/src/test/java/org/apache/flume/node/TestPropertiesFileConfigurationProvider.java > PRE-CREATION > > flume-ng-node/src/test/java/org/apache/flume/source/FlakeySequenceGeneratorSource.java > 41e2f35 > flume-ng-node/src/test/java/org/apache/flume/source/TestNetcatSource.java > 3c17d3d > flume-ng-node/src/test/resources/flume-conf.properties 2b74d4c > > Diff: https://reviews.apache.org/r/7518/diff/ > > > Testing > ------- > > Unit tests added for the new functionality. > > > Thanks, > > Brock Noland > >
