FLUME-632: Support arguments in output format configuration

Review Request #1760 - Created May 22, 2011 and submitted

Information
Jonathan Hsieh
old-flume
flume-632
Reviewers
flume
bruce.mitchener, esammer 
Using the infrastructure setup by flume-635, flume-420, and flume-299, this patch rather trivially adds subargument support for the output format builders.

This patch is not complete, but will be after its dependencies  are resolved.  Specifically, I need to add support to other sinks that use output formats, and more functional tests.



tests on subargs branch pass tests.
Review request changed

Change Summary:

Added code generator and tests.  Still more to do.

Diff:

Revision 2 (+108 -3)

Show changes

flume-core/src/main/java/com/cloudera/flume/conf/FlumeBuilder.java
flume-core/src/main/java/com/cloudera/flume/conf/FlumeSpecGen.java
flume-core/src/main/java/com/cloudera/flume/handlers/debug/ConsoleEventSink.java
flume-core/src/test/java/com/cloudera/flume/conf/TestFlumeBuilder.java
flume-core/src/test/java/com/cloudera/flume/conf/TestFlumeSpecGen.java
Eric Sammer 
  1. My only question is if we need both FormatFactory#{getOutputFormat,createOutputFormat}() or if we can overload one. Non-critical and can be another JIRA to clean up the API if you don't want to deal with it. Just want to reduce our exposed API.
    1. The old API needs to be preserved for backwards compatibility (the "extra work side" of exposing something to plugins).  Overloading causes nastiness with when trying to do function resolution and can result on odd '(Object)' casts.  Will provide example.

I'm going to remove ConsoleEventSink and TestFlumeBuilder changes and add them to a separate review.
  2. nit: spacing.
  3. flume-core/src/main/java/com/cloudera/flume/conf/FlumeSpecGen.java (Diff revision 2) 
     
     
     
     
     
     
     
     
     
     
     
     
    style: I think the preferred method is to not have a surrogate to determine the iteration.

for (int i = 1; i < t.getChildCount(); i++) {
  if (i > 1) {
    sb.append(", ");
  }

  sb.append(genArg((CommonTree)t.getChild(i));
}
  4. nit: spacing
  5. What's the difference between createOutputFormat() and getOutputFormat()? Any reason not to overload one rather than have two that (appear to) do the same thing?
    1. createOutputFormat(String, Object..)
getOutputFormat(String, String...)

They are different names to make calls distinct.  Since Object can auto-coerce to String, and Strings are Object, the resolution of which function is called isn't obvious unless they have different names.

Lets assume both are called getOutputFormat.

Ex: (is the String one called or the Object one?)
getOutputFormat("foo", "arg1", "arg2")
getOutputFormat("foo", "argv1", "arg2") 

Ex: (is the String one called or the Object one?)
getOutputFormat("foo", "arg1", new Object())
getOutputFormat("foo", "argv1", new Object()) 

Ex: (is the String one called or the Object one?)
getOutputFormat("foo")
getOutputFormat("foo") 

  6. 
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 









     

    

   

  

  

   
   Loading...