Flume
  1. Flume
  2. FLUME-1852

Issues with EmbeddedAgentConfiguration

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: v1.4.0
    • Fix Version/s: v1.4.0
    • Component/s: None
    • Labels:
      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

        Issue Links

          Activity

          Will McQueen created issue -
          Brock Noland made changes -
          Field Original Value New Value
          Assignee Brock Noland [ brocknoland ]
          Hide
          Brock Noland added a comment -

          11. We copy the map at the top of the configure method:

          // we are going to modify the properties as we parse the config
          properties = new HashMap<String, String>(properties);

          Show
          Brock Noland added a comment - 11. We copy the map at the top of the configure method: // we are going to modify the properties as we parse the config properties = new HashMap<String, String>(properties);
          Brock Noland made changes -
          Link This issue is duplicated by FLUME-1832 [ FLUME-1832 ]
          Hide
          Brock Noland added a comment -

          #16. I'd actually prefer to keep that comment. We are doing strict checking but I am not sure that is what we want to do. In the case there this causes a problem in the future the reader will know I wasn't sure we should be doing that and remove if appropriate.

          Show
          Brock Noland added a comment - #16. I'd actually prefer to keep that comment. We are doing strict checking but I am not sure that is what we want to do. In the case there this causes a problem in the future the reader will know I wasn't sure we should be doing that and remove if appropriate.
          Brock Noland made changes -
          Attachment FLUME-1852-0.patch [ 12565200 ]
          Brock Noland made changes -
          Remote Link This issue links to "Review Board (Web Link)" [ 11971 ]
          Brock Noland made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Brock Noland made changes -
          Status In Progress [ 3 ] Patch Available [ 10002 ]
          Fix Version/s v1.4.0 [ 12323372 ]
          Hide
          Will McQueen added a comment -

          #16 agreed. Thanks for comment on #11... I see that now.

          Show
          Will McQueen added a comment - #16 agreed. Thanks for comment on #11... I see that now.
          Hide
          Mike Percy added a comment -

          Pushed to trunk and flume-1.4 branches. Thanks Brock!

          Show
          Mike Percy added a comment - Pushed to trunk and flume-1.4 branches. Thanks Brock!
          Mike Percy made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Fix Version/s v1.4.0 [ 12323372 ]
          Resolution Fixed [ 1 ]
          Hide
          Hudson added a comment -

          Integrated in flume-trunk #357 (See https://builds.apache.org/job/flume-trunk/357/)
          FLUME-1852. Issues with EmbeddedAgentConfiguration. (Revision 3df65e12c8d480cd46f190a0bb4addfee4272062)

          Result = SUCCESS
          mpercy : http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=3df65e12c8d480cd46f190a0bb4addfee4272062
          Files :

          • flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/EmbeddedAgentConfiguration.java
          • flume-ng-embedded-agent/src/test/java/org/apache/flume/agent/embedded/TestEmbeddedAgentEmbeddedSource.java
          • flume-ng-embedded-agent/src/test/java/org/apache/flume/agent/embedded/TestEmbeddedAgentConfiguration.java
          • flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/package-info.java
          Show
          Hudson added a comment - Integrated in flume-trunk #357 (See https://builds.apache.org/job/flume-trunk/357/ ) FLUME-1852 . Issues with EmbeddedAgentConfiguration. (Revision 3df65e12c8d480cd46f190a0bb4addfee4272062) Result = SUCCESS mpercy : http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=3df65e12c8d480cd46f190a0bb4addfee4272062 Files : flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/EmbeddedAgentConfiguration.java flume-ng-embedded-agent/src/test/java/org/apache/flume/agent/embedded/TestEmbeddedAgentEmbeddedSource.java flume-ng-embedded-agent/src/test/java/org/apache/flume/agent/embedded/TestEmbeddedAgentConfiguration.java flume-ng-embedded-agent/src/main/java/org/apache/flume/agent/embedded/package-info.java

            People

            • Assignee:
              Brock Noland
              Reporter:
              Will McQueen
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development