Uploaded image for project: 'Flume'
  1. Flume
  2. FLUME-1766

AvroSource throws confusing exception when configured without a port

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 1.3.0
    • Fix Version/s: 1.4.0
    • Component/s: Sinks+Sources
    • Labels:

      Description

      2012-12-03 18:25:08,210 (conf-file-poller-0) [DEBUG - org.apache.flume.source.DefaultSourceFactory.create(DefaultSourceFactory.java:74)] Creating instance of source src-1, type AVRO
      2012-12-03 18:25:08,235 (conf-file-poller-0) [ERROR - org.apache.flume.conf.file.AbstractFileConfigurationProvider$FileWatcherRunnable.run(AbstractFileConfigurationProvider.java:204)] Failed to load configuration data. Exception follows.
      java.lang.NumberFormatException: null
      at java.lang.Integer.parseInt(Integer.java:417)
      at java.lang.Integer.parseInt(Integer.java:499)
      at org.apache.flume.source.AvroSource.configure(AvroSource.java:126)
      at org.apache.flume.conf.Configurables.configure(Configurables.java:41)
      at org.apache.flume.conf.properties.PropertiesFileConfigurationProvider.loadSources(PropertiesFileConfigurationProvider.java:323)
      at org.apache.flume.conf.properties.PropertiesFileConfigurationProvider.load(PropertiesFileConfigurationProvider.java:222)
      at org.apache.flume.conf.file.AbstractFileConfigurationProvider.doLoad(AbstractFileConfigurationProvider.java:123)
      at org.apache.flume.conf.file.AbstractFileConfigurationProvider.access$300(AbstractFileConfigurationProvider.java:38)
      at org.apache.flume.conf.file.AbstractFileConfigurationProvider$FileWatcherRunnable.run(AbstractFileConfigurationProvider.java:202)
      at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:439)
      at java.util.concurrent.FutureTask$Sync.innerRunAndReset(FutureTask.java:317)
      at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:150)
      at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.access$101(ScheduledThreadPoolExecutor.java:98)
      at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.runPeriodic(ScheduledThreadPoolExecutor.java:180)
      at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:204)
      at java.util.concurrent.ThreadPoolExecutor$Worker.runTask(ThreadPoolExecutor.java:886)
      at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:908)
      at java.lang.Thread.run(Thread.java:680)
      ^C2012-12-03 18:25:17,989 (node-shutdownHook) [INFO - org.apache.flume.node.FlumeNode.stop(FlumeNode.java:67)] Flume node stopping - agent

      1. FLUME-1766-3.patch
        2 kB
        Jeff Lord
      2. FLUME-1766-2.patch
        2 kB
        Jeff Lord
      3. FLUME-1766-1.patch
        1 kB
        Jeff Lord

        Activity

        Hide
        jlord Jeff Lord added a comment -

        Local Test Successful Output -

        2012-12-04 19:13:56,553 (conf-file-poller-0) [ERROR - org.apache.flume.node.PollingPropertiesFileConfigurationProvider$FileWatcherRunnable.run(PollingPropertiesFileConfigurationProvider.java:142)] Failed to load configuration data. Exception follows.
        java.lang.IllegalArgumentException: Required parameter port must exist and may not be null

        Show
        jlord Jeff Lord added a comment - Local Test Successful Output - 2012-12-04 19:13:56,553 (conf-file-poller-0) [ERROR - org.apache.flume.node.PollingPropertiesFileConfigurationProvider$FileWatcherRunnable.run(PollingPropertiesFileConfigurationProvider.java:142)] Failed to load configuration data. Exception follows. java.lang.IllegalArgumentException: Required parameter port must exist and may not be null
        Hide
        mpercy Mike Percy added a comment -

        Hi Jeff, thanks for this patch!

        Since you are essentially defining constants (portKey, bindKey), please declare them in all-caps, like the following:

        private final static String PORT_KEY = "port";
        private final static String BIND_KEY = "bind";
        

        and then use those constants in the rest of the configure() method. For example, instead of

        port = context.getInteger("port");
        

        prefer:

        port = context.getInteger(PORT_KEY);
        

        Finally, please make sure your indentation is consistent with the rest of the file. Flume uses 2 spaces everywhere.

        Thanks!

        Show
        mpercy Mike Percy added a comment - Hi Jeff, thanks for this patch! Since you are essentially defining constants (portKey, bindKey), please declare them in all-caps, like the following: private final static String PORT_KEY = "port"; private final static String BIND_KEY = "bind"; and then use those constants in the rest of the configure() method. For example, instead of port = context.getInteger("port"); prefer: port = context.getInteger(PORT_KEY); Finally, please make sure your indentation is consistent with the rest of the file. Flume uses 2 spaces everywhere. Thanks!
        Hide
        hshreedharan Hari Shreedharan added a comment -

        Also, currently bind is not a required value (as of v1.3.0). This patch makes it a required value - which we cannot do in a point release. And frankly, I don't think it should be required, it should be optional as it is now.

        Show
        hshreedharan Hari Shreedharan added a comment - Also, currently bind is not a required value (as of v1.3.0). This patch makes it a required value - which we cannot do in a point release. And frankly, I don't think it should be required, it should be optional as it is now.
        Hide
        jlord Jeff Lord added a comment -

        Ok. Feedback has been incorporated.
        Regarding optional bind, it looks like this value is also assigned to hostname and there is a check in LifecycleSupervisor which looks for that.

        2012-12-04 22:33:04,356 (lifecycleSupervisor-1-3) [ERROR - org.apache.flume.lifecycle.LifecycleSupervisor$MonitorRunnable.run(LifecycleSupervisor.java:238)] Unable to start EventDrivenSourceRunner: { source:Avro source avrosource-1:

        { bindAddress: null, port: 4141 }

        } - Exception follows.
        java.lang.IllegalArgumentException: hostname can't be null

        Show
        jlord Jeff Lord added a comment - Ok. Feedback has been incorporated. Regarding optional bind, it looks like this value is also assigned to hostname and there is a check in LifecycleSupervisor which looks for that. 2012-12-04 22:33:04,356 (lifecycleSupervisor-1-3) [ERROR - org.apache.flume.lifecycle.LifecycleSupervisor$MonitorRunnable.run(LifecycleSupervisor.java:238)] Unable to start EventDrivenSourceRunner: { source:Avro source avrosource-1: { bindAddress: null, port: 4141 } } - Exception follows. java.lang.IllegalArgumentException: hostname can't be null
        Hide
        mpercy Mike Percy added a comment -

        Hari, are you sure?

        Show
        mpercy Mike Percy added a comment - Hari, are you sure?
        Hide
        hshreedharan Hari Shreedharan added a comment -

        It seems like we check for hostname being null in the start method. I checked the configure method where the configuration is validated, so that the source can be removed if the hostname is null (the config system will remove the source if it throws an exception in the configure method). So we should move the check for hostname being non-null to the configure method, so the config system can remove the component when the configure method throws. Jeff - it would be nice to add that to this patch as well. Otherwise the lifecycle supervisor will try to start the component(there is some hack I put in for an error state, so it does not throw every 3 seconds - but I'd rather remove the component than use up resources).

        Show
        hshreedharan Hari Shreedharan added a comment - It seems like we check for hostname being null in the start method. I checked the configure method where the configuration is validated, so that the source can be removed if the hostname is null (the config system will remove the source if it throws an exception in the configure method). So we should move the check for hostname being non-null to the configure method, so the config system can remove the component when the configure method throws. Jeff - it would be nice to add that to this patch as well. Otherwise the lifecycle supervisor will try to start the component(there is some hack I put in for an error state, so it does not throw every 3 seconds - but I'd rather remove the component than use up resources).
        Hide
        hshreedharan Hari Shreedharan added a comment -

        Looks like this is the code in the start method that throws:

            if(maxThreads <= 0) {
              server = new NettyServer(responder,
                      new InetSocketAddress(bindAddress, port));
            } else {
              server = new NettyServer(responder, new InetSocketAddress(bindAddress, port),
                      new NioServerSocketChannelFactory(
                              Executors.newCachedThreadPool(),
                              Executors.newFixedThreadPool(maxThreads)));
            }
        

        Either we should check hostname is not null or not empty in the configure method, or we can set

        bindAddress = InetAddress.getLocalHost().getCanonicalHostName();
        

        Also the LifecycleSupervisor does not check for the hostname, it simply calls start method, which is throwing in this case, which the LifecycleSupervisor logs. LifecycleSupervisor is not really "aware" of any components.

        Show
        hshreedharan Hari Shreedharan added a comment - Looks like this is the code in the start method that throws: if (maxThreads <= 0) { server = new NettyServer(responder, new InetSocketAddress(bindAddress, port)); } else { server = new NettyServer(responder, new InetSocketAddress(bindAddress, port), new NioServerSocketChannelFactory( Executors.newCachedThreadPool(), Executors.newFixedThreadPool(maxThreads))); } Either we should check hostname is not null or not empty in the configure method, or we can set bindAddress = InetAddress.getLocalHost().getCanonicalHostName(); Also the LifecycleSupervisor does not check for the hostname, it simply calls start method, which is throwing in this case, which the LifecycleSupervisor logs. LifecycleSupervisor is not really "aware" of any components.
        Hide
        mpercy Mike Percy added a comment -

        I can see the usability benefit of auto-generating a default. Forcing the user to be explicit about which interface(s) to bind to is good security practice though, IMHO.

        Show
        mpercy Mike Percy added a comment - I can see the usability benefit of auto-generating a default. Forcing the user to be explicit about which interface(s) to bind to is good security practice though, IMHO.
        Hide
        hshreedharan Hari Shreedharan added a comment -

        I don't mind either, but this check must be done in the configure() method, not in start().

        Show
        hshreedharan Hari Shreedharan added a comment - I don't mind either, but this check must be done in the configure() method, not in start().
        Hide
        jlord Jeff Lord added a comment -

        Added back in the check for bindAddress

        Show
        jlord Jeff Lord added a comment - Added back in the check for bindAddress
        Hide
        mpercy Mike Percy added a comment -

        +1, looks good!

        Show
        mpercy Mike Percy added a comment - +1, looks good!
        Hide
        mpercy Mike Percy added a comment -

        Patch committed. Thank you for your contribution, Jeff!

        Trunk rev: aad551d3f45687193ef3323ba6a4584c15c6ad53

        Show
        mpercy Mike Percy added a comment - Patch committed. Thank you for your contribution, Jeff! Trunk rev: aad551d3f45687193ef3323ba6a4584c15c6ad53
        Hide
        brocknoland Brock Noland added a comment -

        Nice work Jeff! Looks like this is missing from trunk?

        Show
        brocknoland Brock Noland added a comment - Nice work Jeff! Looks like this is missing from trunk?
        Hide
        mpercy Mike Percy added a comment -

        Oops i think i mistyped my password when pushing to trunk. (embarrassed)

        Fixed now. Thanks for the catch Brock!

        Show
        mpercy Mike Percy added a comment - Oops i think i mistyped my password when pushing to trunk. (embarrassed) Fixed now. Thanks for the catch Brock!
        Hide
        brocknoland Brock Noland added a comment -

        Thanks!

        Show
        brocknoland Brock Noland added a comment - Thanks!
        Hide
        hudson Hudson added a comment -

        Integrated in flume-trunk #337 (See https://builds.apache.org/job/flume-trunk/337/)
        FLUME-1766. AvroSource throws confusing exception when configured without a port. (Revision aad551d3f45687193ef3323ba6a4584c15c6ad53)

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

        • flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
        Show
        hudson Hudson added a comment - Integrated in flume-trunk #337 (See https://builds.apache.org/job/flume-trunk/337/ ) FLUME-1766 . AvroSource throws confusing exception when configured without a port. (Revision aad551d3f45687193ef3323ba6a4584c15c6ad53) Result = SUCCESS mpercy : http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=aad551d3f45687193ef3323ba6a4584c15c6ad53 Files : flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java

          People

          • Assignee:
            jlord Jeff Lord
            Reporter:
            mpercy Mike Percy
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development