> On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote: > > flume-ng-core/src/main/java/org/apache/flume/channel/DefaultChannelFactory.java, > > line 59 > > <https://reviews.apache.org/r/7518/diff/3/?file=175780#file175780line59> > > > > Looks like an outdated comment?
from the old code > On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote: > > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, > > line 84 > > <https://reviews.apache.org/r/7518/diff/3/?file=175790#file175790line84> > > > > 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. Yeah I don't speed matters here > On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote: > > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, > > line 97 > > <https://reviews.apache.org/r/7518/diff/3/?file=175790#file175790line97> > > > > Nit: This map is cleared at the end of this method after its use, it > > does not need to be cleared now. fixed > On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote: > > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, > > line 117 > > <https://reviews.apache.org/r/7518/diff/3/?file=175790#file175790line117> > > > > 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> > > } +1 > On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote: > > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, > > lines 112-128 > > <https://reviews.apache.org/r/7518/diff/3/?file=175790#file175790line112> > > > > 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 +1 > On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote: > > flume-ng-node/src/main/java/org/apache/flume/node/Application.java, lines > > 283-300 > > <https://reviews.apache.org/r/7518/diff/3/?file=175791#file175791line283> > > > > 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? This is of course the main of Application so we are free to call any method we wish. However, startAllComponents is an internal method (private) and start(),stop() is the for the Application class. If reload is false, we have no additional "utility" LifeCycleAware objects to start. > On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote: > > flume-ng-node/src/main/java/org/apache/flume/node/PollingPropertiesFileConfigurationProvider.java, > > lines 87-95 > > <https://reviews.apache.org/r/7518/diff/3/?file=175797#file175797line87> > > > > We can replace this with: > > while(!executorService.awaitTermination(500, TimeUnit.MILLISECONDS)) { > > <log> > > } That is original code. I restructured it in the next rev because the code above, if interrupted, will spin hard until the executor quits. Now if interrupted we move on without waiting. > On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote: > > flume-ng-node/src/main/java/org/apache/flume/node/PollingPropertiesFileConfigurationProvider.java, > > line 128 > > <https://reviews.apache.org/r/7518/diff/3/?file=175797#file175797line128> > > > > We should probably not loop continously. We should just sleep for 30 > > seconds in between It's executed by a ScheduledExecutorService. Where do you see the continuous loop? > On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote: > > flume-ng-node/src/main/java/org/apache/flume/node/PropertiesFileConfigurationProvider.java, > > line 193 > > <https://reviews.apache.org/r/7518/diff/3/?file=175798#file175798line193> > > > > Is this an old comment? yes, removed > On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote: > > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, > > lines 98-99 > > <https://reviews.apache.org/r/7518/diff/3/?file=175790#file175790line98> > > > > Just to be clear: > > This is meant for reconfiguration right? To take care of old config's > > channels which will no longer be used? Tried to clarify this in the latest patch. - Brock ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7518/#review13760 ----------------------------------------------------------- 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 > d12ad9e > > flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/JdbcChannel.java > 5da979e > > flume-ng-channels/flume-recoverable-memory-channel/src/main/java/org/apache/flume/channel/recoverable/memory/RecoverableMemoryChannel.java > 89296b7 > > 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/annotations/Disposable.java > PRE-CREATION > flume-ng-core/src/main/java/org/apache/flume/annotations/Recyclable.java > PRE-CREATION > > 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 > dfc289e > 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 6a0fe15 > > 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/resources/flume-conf.properties 2b74d4c > > Diff: https://reviews.apache.org/r/7518/diff/ > > > Testing > ------- > > Unit tests added for the new functionality. > > > Thanks, > > Brock Noland > >
