[
https://issues.apache.org/jira/browse/FLUME-978?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13218855#comment-13218855
]
[email protected] commented on FLUME-978:
-----------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/3968/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-02-28 16:49:24)
bq.
bq.
bq. Review request for Flume.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. 1) Changing backing structure of Context from object to String
bq. 2) Add getBoolean, getInteger, getLong to Context
bq. 3) Remove get(key, clazz)
bq. 4) Add javadoc to public methods of Context
bq. 5) Change backing map of Context to synchronized map since it seems this
class could be accessed by multiple threads.
bq.
bq.
bq. This addresses bug FLUME-978.
bq. https://issues.apache.org/jira/browse/FLUME-978
bq.
bq.
bq. Diffs
bq. -----
bq.
bq.
flume-ng-channels/flume-jdbc-channel/src/main/java/org/apache/flume/channel/jdbc/impl/JdbcChannelProviderImpl.java
1cf1c0c
bq. flume-ng-core/src/main/java/org/apache/flume/Context.java f1c8f85
bq.
flume-ng-core/src/main/java/org/apache/flume/channel/ChannelSelectorFactory.java
a7d5f94
bq. flume-ng-core/src/main/java/org/apache/flume/channel/MemoryChannel.java
e79490e
bq.
flume-ng-core/src/main/java/org/apache/flume/channel/MultiplexingChannelSelector.java
c63d0a1
bq.
flume-ng-core/src/main/java/org/apache/flume/channel/PseudoTxnMemoryChannel.java
1df580e
bq. flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5440631
bq. flume-ng-core/src/main/java/org/apache/flume/sink/RollingFileSink.java
45c031d
bq. flume-ng-core/src/main/java/org/apache/flume/sink/SinkGroup.java 1adc5ff
bq.
flume-ng-core/src/main/java/org/apache/flume/sink/SinkProcessorFactory.java
859f4fd
bq. flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
7b079f9
bq. flume-ng-core/src/main/java/org/apache/flume/source/ExecSource.java
a96016c
bq. flume-ng-core/src/main/java/org/apache/flume/source/NetcatSource.java
d205bbc
bq. flume-ng-core/src/test/java/org/apache/flume/TestContext.java a5e6aa8
bq.
flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java
3392dff
bq.
flume-ng-core/src/test/java/org/apache/flume/sink/TestFailoverSinkProcessor.java
93ad3bf
bq.
flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java
b1e67f7
bq.
flume-ng-legacy-sources/flume-thrift-source/src/main/java/org/apache/flume/source/thriftLegacy/ThriftLegacySource.java
5fe270a
bq.
flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java
3da90a5
bq.
flume-ng-sinks/flume-irc-sink/src/main/java/org/apache/flume/sink/irc/IRCSink.java
0a5498f
bq.
bq. Diff: https://reviews.apache.org/r/3968/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Unit tests added for all new methods and some exiting methods.
bq.
bq. All unit tests pass.
bq.
bq.
bq. Thanks,
bq.
bq. Brock
bq.
bq.
> Context interface is too basic requiring boilerplate user code
> --------------------------------------------------------------
>
> Key: FLUME-978
> URL: https://issues.apache.org/jira/browse/FLUME-978
> Project: Flume
> Issue Type: Improvement
> Affects Versions: v1.0.0
> Reporter: Brock Noland
> Assignee: Brock Noland
> Attachments: FLUME-978-0.patch, FLUME-978-1.patch
>
>
> Flume is filled with examples like so:
> batchSize = Integer.parseInt(context.get("batch-size", String.class));
> if (batchSize == null) {
> batchSize = defaultBatchSize;
> }
> from AvroSink. The Context object should provide at a minimum:
> * getBoolean(key)
> * getBoolean(key, default)
> * getInteger(key)
> * getInteger(key, default)
> * getLong(key)
> * getLong(key, default)
> Additionally, the Context object outside of tests, is populated via
> FlumeConfiguration which in the end is a properties file. In this common
> case, all the values in the Context object will be Strings. In this case, if
> we do the obvious and simply provide wrappers for getBoolean, we end up
> executing:
> return Boolean.class.cast(String);
> Users of the Context object should not care where the values come from, only
> that they will be returned the correct object type.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
