Flume, mail # dev - Re: Review Request: FLUME-1630: Flume configuration code could be improved

Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-10-10, 20:59

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/
-----------------------------------------------------------

Review request for Flume.
Description
-------

Patch is not for commit. 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.
This addresses bug FLUME-1630.
   https://issues.apache.org/jira/browse/FLUME-1630
Diffs
-----

  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/Reusable.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 fc3a1e2
  flume-ng-core/src/main/java/org/apache/flume/lifecycle/LifecycleController.java 1e09e2e
  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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-10-11, 00:53

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/
-----------------------------------------------------------

(Updated Oct. 11, 2012, 12:53 a.m.)
Review request for Flume.
Changes
-------

Removes any remaining references to node (except the package) and cleans up some whitespace.
Description
-------

Patch is not for commit. 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.
This addresses bug FLUME-1630.
   https://issues.apache.org/jira/browse/FLUME-1630
Diffs (updated)
-----

  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/Reusable.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 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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-10-11, 00:54

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/
-----------------------------------------------------------

(Updated Oct. 11, 2012, 12:54 a.m.)
Review request for Flume.
Changes
-------

Updated description to clarify intent.
Description (updated)
-------

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.
This addresses bug FLUME-1630.
   https://issues.apache.org/jira/browse/FLUME-1630
Diffs
-----

  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/Reusable.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 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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-10-11, 01:01

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/
-----------------------------------------------------------

(Updated Oct. 11, 2012, 1:01 a.m.)
Review request for Flume.
Changes
-------

Updated description of patch to lay out the big changes that were made.
Description (updated)
-------

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-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/Reusable.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 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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-10-11, 18:58

-----------------------------------------------------------
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.
Changes
-------

Updated patch implements changes discussed on the JIRA.
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 (updated)
-----

  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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-11-20, 12:30

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/#review13622
-----------------------------------------------------------

flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java
<https://reviews.apache.org/r/7518/#comment29203>

whitespace

flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java
<https://reviews.apache.org/r/7518/#comment29202>

missing l on channel
- Brock Noland
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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-11-20, 12:35

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/#review13623
-----------------------------------------------------------

flume-ng-core/src/main/java/org/apache/flume/channel/DefaultChannelFactory.java
<https://reviews.apache.org/r/7518/#comment29204>

Bad comment or is work needed here?
- Brock Noland
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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Hari Shreedharan 2012-11-20, 18:55

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/#review13636
-----------------------------------------------------------
The patch does not apply cleanly. Could you please rebase?

- 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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-11-20, 19:17

> On Nov. 20, 2012, 6:55 p.m., Hari Shreedharan wrote:
> > The patch does not apply cleanly. Could you please rebase?

It was committed to the FLUME-1502 branch which, as I understand it, was so I didn't have to rebase it all the time?
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/#review13636
-----------------------------------------------------------
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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Hari Shreedharan 2012-11-20, 19:24

> On Nov. 20, 2012, 6:55 p.m., Hari Shreedharan wrote:
> > The patch does not apply cleanly. Could you please rebase?
>
> Brock Noland wrote:
> It was committed to the FLUME-1502 branch which, as I understand it, was so I didn't have to rebase it all the time?

I was trying to apply to trunk, I forgot about the branch. So the branch has only this patch right, not the 1502 patch too?
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/#review13636
-----------------------------------------------------------
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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-11-20, 19:32

> On Nov. 20, 2012, 6:55 p.m., Hari Shreedharan wrote:
> > The patch does not apply cleanly. Could you please rebase?
>
> Brock Noland wrote:
> It was committed to the FLUME-1502 branch which, as I understand it, was so I didn't have to rebase it all the time?
>
> Hari Shreedharan wrote:
> I was trying to apply to trunk, I forgot about the branch. So the branch has only this patch right, not the 1502 patch too?

Correct, that has not been committed to the branch yet.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/#review13636
-----------------------------------------------------------
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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Hari Shreedharan 2012-11-27, 22:41

-----------------------------------------------------------
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:

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-11-30, 18:56

> On Nov. 20, 2012, 12:35 p.m., Brock Noland 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>
> >
> > Bad comment or is work needed here?

old comment
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/#review13623
-----------------------------------------------------------
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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-11-30, 18:56

> 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>

yes, removed

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:

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-11-30, 18:56

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/
-----------------------------------------------------------

(Updated Nov. 30, 2012, 6:56 p.m.)
Review request for Flume.
Changes
-------

Update patch based on feedback.
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 (updated)
-----

  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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-11-30, 18:58

> On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote:
> > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, line 325
> > <https://reviews.apache.org/r/7518/diff/3/?file=175790#file175790line325>
> >
> > 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()

I changed how this works so it's more clear.
- 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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-11-30, 19:59

> On Nov. 27, 2012, 10:41 p.m., Hari Shreedharan wrote:
> > 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?

Yes I will address this. Please let me know if there is other feedback as well.
- 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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Brock Noland 2012-11-30, 20:18

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/
-----------------------------------------------------------

(Updated Nov. 30, 2012, 8:18 p.m.)
Review request for Flume.
Changes
-------

I missed the comments of aligning the factories for sources, sinks, and channels. Updated.
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 (updated)
-----

  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

Re: Review Request: FLUME-1630: Flume configuration code could be improved

Hari Shreedharan 2012-11-30, 21:14

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/7518/#review13913
-----------------------------------------------------------

Ship it!
Great work, Brock! Committing this patch, just added one small nit - we can fix it later if you think it makes sense.
flume-ng-node/src/main/java/org/apache/flume/node/PollingPropertiesFileConfigurationProvider.java
<https://reviews.apache.org/r/7518/#comment29720>

Nit: We should probably call shutdownNow() and interrupt the components after a brief period? If you think that is good, we should do that it in a follow up jira
- Hari Shreedharan
On Nov. 30, 2012, 8:18 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/7518/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2012, 8:18 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