You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Brock Noland <br...@cloudera.com> on 2012/02/20 18:55:28 UTC
Review Request: FLUME-978: Context interface is too basic requiring
boilerplate user code
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3968/
-----------------------------------------------------------
Review request for Flume.
Summary
-------
1) Changing backing structure of Context from object to String
2) Add getBoolean, getInteger, getLong to Context
3) Remove get(key, clazz)
4) Add javadoc to public methods of Context
5) Change backing map of Context to synchronized map since it seems this class could be accessed by multiple threads.
This addresses bug FLUME-978.
https://issues.apache.org/jira/browse/FLUME-978
Diffs
-----
flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 0a5498f
flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java b1e67f7
flume-ng-legacy-sources/flume-thrift-source/src/main/java/org/apache/flume/source/thriftLegacy/ThriftLegacySource.java 5fe270a
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 3da90a5
flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java d205bbc
flume-ng-core/src/test/java/org/apache/flume/TestContext.java a5e6aa8
flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java a96016c
flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java 7b079f9
flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 45c031d
flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5440631
flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java 1df580e
flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java c63d0a1
flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java 6a17f06
flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java a7d5f94
flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/impl/JdbcChannelProviderImpl.java 1cf1c0c
flume-ng-core/src/main/java/org/apache/flume/Context.java f1c8f85
Diff: https://reviews.apache.org/r/3968/diff
Testing
-------
Unit tests added for all new methods and some exiting methods.
All unit tests pass.
Thanks,
Brock
Re: Review Request: FLUME-978: Context interface is too basic requiring
boilerplate user code
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3968/#review5435
-----------------------------------------------------------
Ship it!
+1
- Arvind
On 2012-02-29 03:51:07, Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3968/
> -----------------------------------------------------------
>
> (Updated 2012-02-29 03:51:07)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> 1) Changing backing structure of Context from object to String
> 2) Add getBoolean, getInteger, getLong to Context
> 3) Remove get(key, clazz)
> 4) Add javadoc to public methods of Context
> 5) Change backing map of Context to synchronized map since it seems this class could be accessed by multiple threads.
>
>
> This addresses bug FLUME-978.
> https://issues.apache.org/jira/browse/FLUME-978
>
>
> Diffs
> -----
>
> flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/impl/JdbcChannelProviderImpl.java 1cf1c0c
> flume-ng-core/src/main/java/org/apache/flume/Context.java f1c8f85
> flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java a7d5f94
> flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java e79490e
> flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java c63d0a1
> flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java 1df580e
> flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5440631
> flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 45c031d
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 1adc5ff
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 859f4fd
> flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java 7b079f9
> flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java a96016c
> flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java d205bbc
> flume-ng-core/src/test/java/org/apache/flume/TestContext.java a5e6aa8
> flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java 3392dff
> flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java 93ad3bf
> flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java b1e67f7
> flume-ng-legacy-sources/flume-thrift-source/src/main/java/org/apache/flume/source/thriftLegacy/ThriftLegacySource.java 5fe270a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 3da90a5
> flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 0a5498f
>
> Diff: https://reviews.apache.org/r/3968/diff
>
>
> Testing
> -------
>
> Unit tests added for all new methods and some exiting methods.
>
> All unit tests pass.
>
>
> Thanks,
>
> Brock
>
>
Re: Review Request: FLUME-978: Context interface is too basic requiring
boilerplate user code
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3968/
-----------------------------------------------------------
(Updated 2012-02-29 03:51:07.275144)
Review request for Flume.
Changes
-------
Updated based on review.
Summary
-------
1) Changing backing structure of Context from object to String
2) Add getBoolean, getInteger, getLong to Context
3) Remove get(key, clazz)
4) Add javadoc to public methods of Context
5) Change backing map of Context to synchronized map since it seems this class could be accessed by multiple threads.
This addresses bug FLUME-978.
https://issues.apache.org/jira/browse/FLUME-978
Diffs (updated)
-----
flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/impl/JdbcChannelProviderImpl.java 1cf1c0c
flume-ng-core/src/main/java/org/apache/flume/Context.java f1c8f85
flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java a7d5f94
flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java e79490e
flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java c63d0a1
flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java 1df580e
flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5440631
flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 45c031d
flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 1adc5ff
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 859f4fd
flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java 7b079f9
flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java a96016c
flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java d205bbc
flume-ng-core/src/test/java/org/apache/flume/TestContext.java a5e6aa8
flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java 3392dff
flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java 93ad3bf
flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java b1e67f7
flume-ng-legacy-sources/flume-thrift-source/src/main/java/org/apache/flume/source/thriftLegacy/ThriftLegacySource.java 5fe270a
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 3da90a5
flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 0a5498f
Diff: https://reviews.apache.org/r/3968/diff
Testing
-------
Unit tests added for all new methods and some exiting methods.
All unit tests pass.
Thanks,
Brock
Re: Review Request: FLUME-978: Context interface is too basic requiring
boilerplate user code
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3968/#review5428
-----------------------------------------------------------
For some reason the following comment did not show up in my last review. Submitting it again.
flume-ng-core/src/main/java/org/apache/flume/Context.java
<https://reviews.apache.org/r/3968/#comment11831>
This call should be wrapped in synchronized(parameters) block as it may iterate over the contents of the map.
- Arvind
On 2012-02-28 16:49:24, Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3968/
> -----------------------------------------------------------
>
> (Updated 2012-02-28 16:49:24)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> 1) Changing backing structure of Context from object to String
> 2) Add getBoolean, getInteger, getLong to Context
> 3) Remove get(key, clazz)
> 4) Add javadoc to public methods of Context
> 5) Change backing map of Context to synchronized map since it seems this class could be accessed by multiple threads.
>
>
> This addresses bug FLUME-978.
> https://issues.apache.org/jira/browse/FLUME-978
>
>
> Diffs
> -----
>
> flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/impl/JdbcChannelProviderImpl.java 1cf1c0c
> flume-ng-core/src/main/java/org/apache/flume/Context.java f1c8f85
> flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java a7d5f94
> flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java e79490e
> flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java c63d0a1
> flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java 1df580e
> flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5440631
> flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 45c031d
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 1adc5ff
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 859f4fd
> flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java 7b079f9
> flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java a96016c
> flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java d205bbc
> flume-ng-core/src/test/java/org/apache/flume/TestContext.java a5e6aa8
> flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java 3392dff
> flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java 93ad3bf
> flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java b1e67f7
> flume-ng-legacy-sources/flume-thrift-source/src/main/java/org/apache/flume/source/thriftLegacy/ThriftLegacySource.java 5fe270a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 3da90a5
> flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 0a5498f
>
> Diff: https://reviews.apache.org/r/3968/diff
>
>
> Testing
> -------
>
> Unit tests added for all new methods and some exiting methods.
>
> All unit tests pass.
>
>
> Thanks,
>
> Brock
>
>
Re: Review Request: FLUME-978: Context interface is too basic requiring
boilerplate user code
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3968/#review5427
-----------------------------------------------------------
Thanks for the patch Brock. Changes look good. A couple of suggestions:
flume-ng-core/src/main/java/org/apache/flume/Context.java
<https://reviews.apache.org/r/3968/#comment11830>
Another thing to consider while you are at it is to provide a constructor that takes in a Map<String, String>. That way the two step call:
Context c = new Context();
c.putAll(config);
Can be collapsed into one:
Context c = new Context(config);
If you decide to do this, I suggest making a copy of the map that is passed in to insulate it from accidental external modification.
flume-ng-core/src/main/java/org/apache/flume/Context.java
<https://reviews.apache.org/r/3968/#comment11829>
Since the parameters member variable is used for synchronization, you cannot replace it's reference. Replacing it's reference will break the thread safety of this class.
Alternatively, you can instantiate parameters at the point of declaration or in constructor, and the clear() could then be applied to it directly.
- Arvind
On 2012-02-28 16:49:24, Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3968/
> -----------------------------------------------------------
>
> (Updated 2012-02-28 16:49:24)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> 1) Changing backing structure of Context from object to String
> 2) Add getBoolean, getInteger, getLong to Context
> 3) Remove get(key, clazz)
> 4) Add javadoc to public methods of Context
> 5) Change backing map of Context to synchronized map since it seems this class could be accessed by multiple threads.
>
>
> This addresses bug FLUME-978.
> https://issues.apache.org/jira/browse/FLUME-978
>
>
> Diffs
> -----
>
> flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/impl/JdbcChannelProviderImpl.java 1cf1c0c
> flume-ng-core/src/main/java/org/apache/flume/Context.java f1c8f85
> flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java a7d5f94
> flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java e79490e
> flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java c63d0a1
> flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java 1df580e
> flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5440631
> flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 45c031d
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 1adc5ff
> flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 859f4fd
> flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java 7b079f9
> flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java a96016c
> flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java d205bbc
> flume-ng-core/src/test/java/org/apache/flume/TestContext.java a5e6aa8
> flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java 3392dff
> flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java 93ad3bf
> flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java b1e67f7
> flume-ng-legacy-sources/flume-thrift-source/src/main/java/org/apache/flume/source/thriftLegacy/ThriftLegacySource.java 5fe270a
> flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 3da90a5
> flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 0a5498f
>
> Diff: https://reviews.apache.org/r/3968/diff
>
>
> Testing
> -------
>
> Unit tests added for all new methods and some exiting methods.
>
> All unit tests pass.
>
>
> Thanks,
>
> Brock
>
>
Re: Review Request: FLUME-978: Context interface is too basic requiring
boilerplate user code
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3968/
-----------------------------------------------------------
(Updated 2012-02-28 16:49:24.308965)
Review request for Flume.
Changes
-------
Updated patch due to trunk commits.
Summary
-------
1) Changing backing structure of Context from object to String
2) Add getBoolean, getInteger, getLong to Context
3) Remove get(key, clazz)
4) Add javadoc to public methods of Context
5) Change backing map of Context to synchronized map since it seems this class could be accessed by multiple threads.
This addresses bug FLUME-978.
https://issues.apache.org/jira/browse/FLUME-978
Diffs (updated)
-----
flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/impl/JdbcChannelProviderImpl.java 1cf1c0c
flume-ng-core/src/main/java/org/apache/flume/Context.java f1c8f85
flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java a7d5f94
flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java e79490e
flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java c63d0a1
flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java 1df580e
flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5440631
flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java 45c031d
flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 1adc5ff
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java 859f4fd
flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java 7b079f9
flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java a96016c
flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java d205bbc
flume-ng-core/src/test/java/org/apache/flume/TestContext.java a5e6aa8
flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java 3392dff
flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java 93ad3bf
flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java b1e67f7
flume-ng-legacy-sources/flume-thrift-source/src/main/java/org/apache/flume/source/thriftLegacy/ThriftLegacySource.java 5fe270a
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 3da90a5
flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java 0a5498f
Diff: https://reviews.apache.org/r/3968/diff
Testing
-------
Unit tests added for all new methods and some exiting methods.
All unit tests pass.
Thanks,
Brock