Details
-
Bug
-
Status: Resolved
-
Major
-
Resolution: Fixed
-
1.4.0
-
None
-
None
Description
Hi,
Could you please provide feedback on the following points?
In EmbeddedAgentConfiguration.java:
1. Comment says "Source type, choices are 'embedded' or 'avro'
//fix: should only be 'embedded'
2. 'embedded' doesn't work as a value of 'source.type'. Either the 'source.type' line should be ommitted in the config (so that it defaults to the proper class), or the source.type needs to say:
"source.type=org.apache.flume.agent.embedded.EmbeddedSource"
Ideally, it would be nice if a user actually could specify "embedded" as an alias for the entire fully-qualified class name. I see that we don't include EMBEDDED in the SourceType class, maybe intentionally so as not to confuse an embedded-agent-specific type with generic flume agent types. In any case, the Flume User Guide says that the default value for source.type is "embedded".
3. Search for "pulAll" in comments. Should be "putAll".
4. Search for "recommended source" in comments. Should be "only source".
5. Search for "File based channel which stores events in heap" in comments. Should change 'in heap' to 'on local disk'.
6. Search for "Choices are 'default' (failover) or 'load_balance'". AFAIK, "default", "failover", and "load_balance" are 3 separate sink processor types, and so 'default' is not the same as 'failover'. The ALLOWED_SINK_PROCESSORS array seems to support this thinking since it also 3 entries.
8. In configure(), why do we define "Joiner joiner = Joiner.on(SEPARATOR)" since the JOINER constant has already be defined. Would it make sense just to use the constant instead?
9. "point agent at source" comment is repeated twice. The second time it should be "point agent at sinks"
10. "the the" => "the"
11. Search for "properties.remove(key)". Wouldn't this give an error if the user passes-in an ImmutableMap instance to EmbeddedAgent.configure(..), which eventually delegates that same map to EmbeddedAgentConfiguration.configure(..)? I imagine that it's entirely reasonable for a user to do this, especially given a functional programming mindset.
12. Search for "key.startsWith(sink)". Should be "key.startsWith(sink+SEPARATOR)". Otherwise, if you have for example 11 sinks numbered k1 through k11, then your 'key' var might point to value "k1", and then startsWith("k1") could find k11's prop values and store those into the k1 sink so that k1 sink would actually have some or all of k11's values it seems.
13. Similarly, please do search-and-replace for:
key.startsWith(SOURCE) => key.startsWith(SOURCE_PREFIX) .. (1 place)
key.startsWith(CHANNEL) => key.startsWith(CHANNEL_PREFIX) .. (1 place)
key.startsWith(SINK_PROCESSOR) => key.startsWith(SINK_PROCESSOR_PREFIX) .. (1 place)
14. Please do search-and-replace for:
key.replace(SOURCE, sourceName) => key.replaceFirst(SOURCE, sourceName)
key.replace(CHANNEL, channelName) => key.replaceFirst(CHANNEL, channelName)
...since the intent is to replace only the first instance rather than all of them, right?
15. The strings in the embedded agent's original config file that start with user-defined chars are the sink aliases like k1, k2, etc. All other lines must begin with one of "source", "channel", "processor", or "sinks". From what I can tell looking at the EmbeddedAgentConfiguration code, a user must not specify a sink alias name that matches one "source", "channel", or "processor" strings.. otherwise EmbeddedAgentConfiguration would seem to have issues.
16. There's an "XXX" comment that shows that we throw a FlumeException when the user-specified embedded agent config contains an unexpected prop. The comment say "XXX should we simply ignore this?" meaning that we're currently doing strict checking of the user-provided props. If we plan to continue doing this, could we please remove the comment?
Thank you 
Attachments
Attachments
Issue Links
- is duplicated by
-
-
- Resolved
-
- links to
