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

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

Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
This addresses bug FLUME-1772.
    https://issues.apache.org/jira/browse/FLUME-1772
Diffs
-----

  flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
  flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1

Diff: https://reviews.apache.org/r/8421/diff/
Testing
-------

Tests added for source, channel, and sink.
Thanks,

Brock Noland

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14202
-----------------------------------------------------------
Generally looks good. I have a couple of comments:
* There are a lot of tabs/trailing spaces. Could you please remove those.
* If we remove these components, we need to make sure that after all components are loaded/removed, we have at least 1 channel with a source or sink in the agent. Basically after all load* methods are done, we should iterate through the sinks and sources and verify their channels(in case of sources, at least one of the channels) are still around (if not the source/sink should be removed), else the config is invalid.

- Hari Shreedharan
On Dec. 7, 2012, 8:37 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2012, 8:37 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 8, 2012, 12:39 a.m., Hari Shreedharan wrote:
> > Generally looks good. I have a couple of comments:
> > * There are a lot of tabs/trailing spaces. Could you please remove those.
> > * If we remove these components, we need to make sure that after all components are loaded/removed, we have at least 1 channel with a source or sink in the agent. Basically after all load* methods are done, we should iterate through the sinks and sources and verify their channels(in case of sources, at least one of the channels) are still around (if not the source/sink should be removed), else the config is invalid.

Also after iterating, there must be at least one source or sink remaining.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14202
-----------------------------------------------------------
On Dec. 7, 2012, 8:37 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2012, 8:37 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 8, 2012, 12:39 a.m., Hari Shreedharan wrote:
> > Generally looks good. I have a couple of comments:
> > * There are a lot of tabs/trailing spaces. Could you please remove those.
> > * If we remove these components, we need to make sure that after all components are loaded/removed, we have at least 1 channel with a source or sink in the agent. Basically after all load* methods are done, we should iterate through the sinks and sources and verify their channels(in case of sources, at least one of the channels) are still around (if not the source/sink should be removed), else the config is invalid.
>
> Hari Shreedharan wrote:
>     Also after iterating, there must be at least one source or sink remaining.

Thanks Hari, sorry for all the spaces. I thought I had removed them.

Regarding the sources/sinks. That is a good point....I wonder about implementation though. For example, often for testing we might configure a source and channel with no sink. Or in production after having some issues with a file channel the data may be moved aside and after some debugging a sink and channel hooked together to put the data back into the system.  What are your thoughts about these cases?
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14202
-----------------------------------------------------------
On Dec. 7, 2012, 8:37 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2012, 8:37 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 8, 2012, 12:39 a.m., Hari Shreedharan wrote:
> > Generally looks good. I have a couple of comments:
> > * There are a lot of tabs/trailing spaces. Could you please remove those.
> > * If we remove these components, we need to make sure that after all components are loaded/removed, we have at least 1 channel with a source or sink in the agent. Basically after all load* methods are done, we should iterate through the sinks and sources and verify their channels(in case of sources, at least one of the channels) are still around (if not the source/sink should be removed), else the config is invalid.
>
> Hari Shreedharan wrote:
>     Also after iterating, there must be at least one source or sink remaining.
>
> Brock Noland wrote:
>     Thanks Hari, sorry for all the spaces. I thought I had removed them.
>    
>     Regarding the sources/sinks. That is a good point....I wonder about implementation though. For example, often for testing we might configure a source and channel with no sink. Or in production after having some issues with a file channel the data may be moved aside and after some debugging a sink and channel hooked together to put the data back into the system.  What are your thoughts about these cases?

I would assume it should be something like this:

* Iterate through the channels and configure them - removing any badly configured ones
* Iterate through sources - first check if the channel is still around, if it is not - remove, else configure. If it throws exception, remove it. Same for sinks.
* Now iterate through the channels once again to check there is at least one sink or source (maybe we should keep a counter or map of components connected to the channels)
* Finally, verify there is at least one channel (by induction, there is at least one sink or source connected to it).

FYI - Something similar is done in FlumeConfiguration.java, but not exactly the same though.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14202
-----------------------------------------------------------
On Dec. 7, 2012, 8:37 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2012, 8:37 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 8, 2012, 12:39 a.m., Hari Shreedharan wrote:
> > Generally looks good. I have a couple of comments:
> > * There are a lot of tabs/trailing spaces. Could you please remove those.
> > * If we remove these components, we need to make sure that after all components are loaded/removed, we have at least 1 channel with a source or sink in the agent. Basically after all load* methods are done, we should iterate through the sinks and sources and verify their channels(in case of sources, at least one of the channels) are still around (if not the source/sink should be removed), else the config is invalid.
>
> Hari Shreedharan wrote:
>     Also after iterating, there must be at least one source or sink remaining.
>
> Brock Noland wrote:
>     Thanks Hari, sorry for all the spaces. I thought I had removed them.
>    
>     Regarding the sources/sinks. That is a good point....I wonder about implementation though. For example, often for testing we might configure a source and channel with no sink. Or in production after having some issues with a file channel the data may be moved aside and after some debugging a sink and channel hooked together to put the data back into the system.  What are your thoughts about these cases?
>
> Hari Shreedharan wrote:
>     I would assume it should be something like this:
>    
>     * Iterate through the channels and configure them - removing any badly configured ones
>     * Iterate through sources - first check if the channel is still around, if it is not - remove, else configure. If it throws exception, remove it. Same for sinks.
>     * Now iterate through the channels once again to check there is at least one sink or source (maybe we should keep a counter or map of components connected to the channels)
>     * Finally, verify there is at least one channel (by induction, there is at least one sink or source connected to it).
>    
>     FYI - Something similar is done in FlumeConfiguration.java, but not exactly the same though.

My mistake, I read "source or sink" as "source and sink"
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14202
-----------------------------------------------------------
On Dec. 7, 2012, 8:37 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2012, 8:37 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

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

(Updated Dec. 12, 2012, 7:30 p.m.)
Review request for Flume.
Changes
-------

Latest patch which addresses Hari's concerns.
Description
-------

Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
This addresses bug FLUME-1772.
    https://issues.apache.org/jira/browse/FLUME-1772
Diffs (updated)
-----

  flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
  flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1

Diff: https://reviews.apache.org/r/8421/diff/
Testing
-------

Tests added for source, channel, and sink.
Thanks,

Brock Noland

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

flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java
<https://reviews.apache.org/r/8421/#comment31139>

    If channels list is empty should we throw? I think we should just remove the source and log it right?

flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java
<https://reviews.apache.org/r/8421/#comment31140>

    Same as above, should we throw or remove the component?

flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java
<https://reviews.apache.org/r/8421/#comment31141>

    same as above

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

    should be called testSourceAndSinkThrowExceptionDuringConfiguration()
- Hari Shreedharan
On Dec. 12, 2012, 7:30 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 7:30 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 18, 2012, 8:34 p.m., Hari Shreedharan wrote:
> > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, line 340
> > <https://reviews.apache.org/r/8421/diff/2/?file=237476#file237476line340>
> >
> >     If channels list is empty should we throw? I think we should just remove the source and log it right?

FlumeConfiguration should have already eliminated these, correct? If not, wouldn't it be a bug in FlumeConfiguration?
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14378
-----------------------------------------------------------
On Dec. 12, 2012, 7:30 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 7:30 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 18, 2012, 8:34 p.m., Hari Shreedharan wrote:
> > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, line 340
> > <https://reviews.apache.org/r/8421/diff/2/?file=237476#file237476line340>
> >
> >     If channels list is empty should we throw? I think we should just remove the source and log it right?
>
> Brock Noland wrote:
>     FlumeConfiguration should have already eliminated these, correct? If not, wouldn't it be a bug in FlumeConfiguration?

If these channels were removed/not added because the configure method threw, this could still be empty even if FlumeConfiguration did not remove it.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14378
-----------------------------------------------------------
On Dec. 12, 2012, 7:30 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 7:30 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 18, 2012, 8:34 p.m., Hari Shreedharan wrote:
> > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, line 340
> > <https://reviews.apache.org/r/8421/diff/2/?file=237476#file237476line340>
> >
> >     If channels list is empty should we throw? I think we should just remove the source and log it right?
>
> Brock Noland wrote:
>     FlumeConfiguration should have already eliminated these, correct? If not, wouldn't it be a bug in FlumeConfiguration?
>
> Hari Shreedharan wrote:
>     If these channels were removed/not added because the configure method threw, this could still be empty even if FlumeConfiguration did not remove it.

Look a few lines below, we catch the exception and log it.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14378
-----------------------------------------------------------
On Dec. 12, 2012, 7:30 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2012, 7:30 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 18, 2012, 8:34 p.m., Hari Shreedharan wrote:
> > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, line 340
> > <https://reviews.apache.org/r/8421/diff/2/?file=237476#file237476line340>
> >
> >     If channels list is empty should we throw? I think we should just remove the source and log it right?
>
> Brock Noland wrote:
>     FlumeConfiguration should have already eliminated these, correct? If not, wouldn't it be a bug in FlumeConfiguration?
>
> Hari Shreedharan wrote:
>     If these channels were removed/not added because the configure method threw, this could still be empty even if FlumeConfiguration did not remove it.
>
> Brock Noland wrote:
>     Look a few lines below, we catch the exception and log it.

I just updated the test name. I think this should be fine since we catch the exception.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14378
-----------------------------------------------------------
On Dec. 18, 2012, 9:11 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2012, 9:11 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 18, 2012, 8:34 p.m., Hari Shreedharan wrote:
> > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, line 340
> > <https://reviews.apache.org/r/8421/diff/2/?file=237476#file237476line340>
> >
> >     If channels list is empty should we throw? I think we should just remove the source and log it right?
>
> Brock Noland wrote:
>     FlumeConfiguration should have already eliminated these, correct? If not, wouldn't it be a bug in FlumeConfiguration?
>
> Hari Shreedharan wrote:
>     If these channels were removed/not added because the configure method threw, this could still be empty even if FlumeConfiguration did not remove it.
>
> Brock Noland wrote:
>     Look a few lines below, we catch the exception and log it.
>
> Brock Noland wrote:
>     I just updated the test name. I think this should be fine since we catch the exception.

Ah ok. looks good.
- Hari
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14378
-----------------------------------------------------------
On Dec. 18, 2012, 9:11 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2012, 9:11 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

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

(Updated Dec. 18, 2012, 9:11 p.m.)
Review request for Flume.
Changes
-------

Updated test name.
Description
-------

Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
This addresses bug FLUME-1772.
    https://issues.apache.org/jira/browse/FLUME-1772
Diffs (updated)
-----

  flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
  flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1

Diff: https://reviews.apache.org/r/8421/diff/
Testing
-------

Tests added for source, channel, and sink.
Thanks,

Brock Noland

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

Thanks for the updated patch. Just one thing I missed last time.
flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java
<https://reviews.apache.org/r/8421/#comment31179>

    Sorry, missed this one last time. This channel should also be removed from the channelCache, so we can remove the reference to this channel.
- Hari Shreedharan
On Dec. 18, 2012, 9:11 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2012, 9:11 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

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

flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java
<https://reviews.apache.org/r/8421/#comment31183>

    Good catch!
- Brock Noland
On Dec. 18, 2012, 9:11 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2012, 9:11 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 19, 2012, 1:15 a.m., Brock Noland wrote:
> > flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java, line 114
> > <https://reviews.apache.org/r/8421/diff/3/?file=240770#file240770line114>
> >
> >     Good catch!

I'll update this patch tomorrow as I am testing another patch.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14705
-----------------------------------------------------------
On Dec. 18, 2012, 9:11 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2012, 9:11 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

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

(Updated Dec. 19, 2012, 4:26 p.m.)
Review request for Flume.
Changes
-------

Updated based on Hari's comments.
Description
-------

Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
This addresses bug FLUME-1772.
    https://issues.apache.org/jira/browse/FLUME-1772
Diffs (updated)
-----

  flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
  flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1

Diff: https://reviews.apache.org/r/8421/diff/
Testing
-------

Tests added for source, channel, and sink.
Thanks,

Brock Noland

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

Generally looks good. I have a couple of questions though:

* The components list in ChannelComponent class seems to be not used. The only time it is actually used is to check if it is empty (otherwise the components are simply added to it). I think this list can easily be replaced by a boolean right?
* Assume that the components is required for something (which I missed), then we need 2 lists for sources and sinks, since we can have a source and a sink with the same name.

- Hari Shreedharan
On Dec. 19, 2012, 4:26 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2012, 4:26 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

> On Dec. 20, 2012, 2:19 a.m., Hari Shreedharan wrote:
> > Brock,
> >
> > Generally looks good. I have a couple of questions though:
> >
> > * The components list in ChannelComponent class seems to be not used. The only time it is actually used is to check if it is empty (otherwise the components are simply added to it). I think this list can easily be replaced by a boolean right?
> > * Assume that the components is required for something (which I missed), then we need 2 lists for sources and sinks, since we can have a source and a sink with the same name.

Hi,

Thanks for you review! Regarding components, it is there to serve the purpose of the flag but also gives us a nice log message. Since we only care that a source or a sink is connected to the channel I don't think we need two lists.

Brock
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/8421/#review14743
-----------------------------------------------------------
On Dec. 19, 2012, 4:26 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2012, 4:26 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>

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

Ship it!
Ship It!

- Hari Shreedharan
On Dec. 19, 2012, 4:26 p.m., Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/8421/
> -----------------------------------------------------------
>
> (Updated Dec. 19, 2012, 4:26 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Catches Exception around all the configure() methods of components. If they throw an exception, they are removed from the configuration (not added).
>
>
> This addresses bug FLUME-1772.
>     https://issues.apache.org/jira/browse/FLUME-1772
>
>
> Diffs
> -----
>
>   flume-ng-node/src/main/java/org/apache/flume/node/AbstractConfigurationProvider.java daef76b
>   flume-ng-node/src/test/java/org/apache/flume/node/TestAbstractConfigurationProvider.java 25001b1
>
> Diff: https://reviews.apache.org/r/8421/diff/
>
>
> Testing
> -------
>
> Tests added for source, channel, and sink.
>
>
> Thanks,
>
> Brock Noland
>
>