FLUME 136: Port Configuration Server to Avro

Review Request #319 - Created July 13, 2010 and submitted

Patrick Wendell
Henry, jon, phunt
These are some preliminary files for the flume conversion to Avro. This review consists of three types of files

1) Avro source files and build targets
2) FlumeConfigData, a class capable of translating itself to/from Avro and Flume data types.
3) Avro versions of existing client/server implementations.

Having the translation logic within the intermediary class (FlumeConfigData) may not be the best way to do it. However, there isn't really an obvious place to put this logic. We could put it *just* in the client or server implementations, but then it's awkward to import translating logic from a server to a client or vice-versa. I'm open to suggestions though!

This upgrade is in two github commits on my github:
Avro port: eb751f9038d8098c9971d868da07949b33c49f35
See TestAvroMultiMasterRPC.java for an end-to-end test of client-server communication with Avro.
  1. This is pretty awesome, nice work. 
    My suggestions for improvement are as follows:
    1. More test coverage - ideally you should at least make every call from the client and check that the mock server gets what it expects
    2. Reduce code duplication - if it's at all possible to wrap out some repeated code from *MultiMasterRPC that would be superb, because that Retryable stuff is so hairy I don't want to have to maintain two identical versions. 
    3. Make sure you've got copyright headers everywhere (copy them from any other source file), and at least give every class a javadoc comment.
    4. There's no way of switching between Avro and Thrift, right? Do you want to add that in a separate patch? 
    Let me know what you think about the above, and we'll be pretty close to thinking about integrating. 
    1. One overarching question is: Do we want to continue to support *both* Avro and Thrift or are we drawing a line in the sand and porting things over to Avro from now own. I've been assuming the second case and that's why there isn't much focus on avoiding duplication, because I assumed we are going to abandon the old stuff.
      From what I'm hearing, it sounds like we do want to support both going forward (?) Is that correct? It would be helpful to have a clear intent here, so that I don't get stuck in a middle ground in terms of the degree of abstraction we are providing for RPC stuff.
  2. ivy.xml (Diff revision 1)
    out of interest, why remove this?
    1. I had inserted this earlier but it wasn't necessary, just removing it in this rev.
  3. src/avro/flumeconfig.avdl (Diff revision 1)
    Can avdl files have comments? If so, these need some documentation, and a copyright / license header. 
  4. Need a license header
  5. you don't set trans to null in close() so this condition will only get triggered if you call close before open. 
  6. seems like a race condition is possible here if two calls into ensureConnected are made simultaneously. If this is always called under the same lock, you should document that in a comment, 
  7. again, license header
  8. This class seems fine, but are you sure there's no way to avoid repeating all the code from ThriftMultiMasterRPC? What if there was a 'MultiMasterRPC' class that wrapped either Thrift or Avro rather than inherited from it?
  9. Hm, I wonder here whether having an explicit dependency from this class onto both Thrift and Avro is the right thing to do. It'd be nice to have it easy not ship Thrift by excluding some classes. 
    Instead you could have an AvroFlumeConfigDataConverter with a couple of static methods 'fromAvro' and 'toAvro', and similarly for Thrift. Then you decouple FlumeConfigData from any knowledge of possible serialisation types. 
  10. As tedious as it sounds, these constructors needs testing to make sure you don't miss a parameter somewhere. 
  11. you should use JUnit4-style test cases (no way you should have known that though). remove 'extends TestCase', and add import org.junit.*;
    Then to each test method add the @Test annotation.
  1. "what henry said" plus:
    Does this assume you have Avro installed or something like that? The DEVNOTES files should be updated with a section for avro similar to what we have there for thrift.
    Any other documentation changes needed?
    1. Actually, no. Unlike Thrift all of the Avro functionality that we need is contained in the JAR file. Avro actually defines ant tasks that deal with pre-compilation, so it's pretty much drop-in to existing Flume build process.
    2. you should mention to Sam, he'll be psyched (was bitching about thrift being a pita re flume compilation the last time I talked to him) :-)
  2. src/avro/flumeconfig.avdl (Diff revision 1)
    in general seems like we should have the same/simliar comments as currently exist in thrift version of this file.
    1. noted, was waiting for doug on some info about how to add comments in these files.
  3. not necessary to say .getName()
  4. prefer javadoc over comment for things like this - also useful when browsing code in eclipse (adds to the tooltip)
  5. my personal pref here would be to wrap the original exception rather than the message. losing valuable information.
    check with Henry/Jon though, not sure their preference.
  6. good to take out bogus todos
  1. When you say "rewritten for avro version of jetty" what does that mean? I see some changes to the code, but no changes to lib/jetty? btw, I ported Flume over to jetty 6 as part of my hbase sink work - perhaps we should extract that, apply to flume master (new jira/patch) and then base your avro changes off that branch? would allow you to focus on avro specifically. LMK if you want to work together on that.
    1. Ya so there are some files that aren't included in this review. This includes the one line change to the jetty lib and also about 30 files in which I change the import path (since some refactoring of FlumeConfigData was done). It was a total mess to put them all in this one review, so the trivial changes I left out and we can either review subsequently or you can just look at them on github before pulling the changes in. What's the best way to handle this situation?
      I just quickly ported the StatusHTTPServer to Jetty be 6 compatible... happy to defer to any porting you've already done. I'm not jetty expert so it's likely yours is better.
    2. haha, neither am I (jetty expert). sync up with me and we'll discuss and come up with a plan.
  1. This is quite a bit and a lot of it looks really good.  I have a few questions about the api decisions you made in the refactor --
    Why did open/close change?  
    Not sure we need the distinction of a single master vs master.  The api difference doesn't seem intuitive to me -- why not in the constructor instead?
    For future reviews, it may be nice to break up into separate reviews -- even if it just at the file granularity.
    Here is one suggested way of breaking them up;
    1) StatusHttpServer
    2) Avro related pieces
    3) Thrift related pieces
    4) The report server stuff.
    I haven't looked at the previous comments yet, and want to do that and another pass before the ship it.
    1. Answers:
      1) Open was never used externally in the previous implementation of MasterRPC. Also, it's not entirely clear what it means in this context. The Avro client doesn't ever connect to the server until an RPC function is called, so open() wouldn't do anything - in fact, it could deceptively make the user think Avro has connected successfully to a server if open() simply does nothing and silently returns. To avoid this confusion, since open() is not universal to all RPC implementations, and was never used, I removed it from the interface.
      2) If we remove SingleMaster, then a MutliMasterRPC could itself wrap another MultiMasterRPC- that seemed strange to me. It seemed more explicit to make a formal extension which handles connections to only one master, such that someone adding a third RPC framework knows they simply need to add another implementation of *that* interface.
      3) Will try to break up in future. This review excluded about 30 files which had one-line changes to the import path for FlumeConfigData, since I refactored that class, so that much I did. However, your proposed breakup looks better.
  2. src/avro/flumeconfig.avdl (Diff revision 2)
    copyright? (is this autogen?)
  3. copyright
  4. Does init actually grab resources? (bind to ports / create network sockets?)
  5. I prefer this going into the constructor (it never changes right?)
    My general preference is to allow complicated constructors, and to keep the api clean and simple.
  6. src/java/com/cloudera/flume/agent/AvroMasterRPC.java (Diff revision 2)
    And, along with the previous preference, this goes into a no argument open/init method.
  7. does this block until resources are given up?
  8. put error message as second argument to preconditions (assume will be user visible).
  9. add e as second argument so stack traces are still available?
  10. e as second arg?
  11. @Override
  12. Whoa, where happened to open and close?
    why no exceptions?
    1. Can you give an example of a reason when someone would do anything other than ignore an exception on a close(). Guess I'm just now sure what the use case is for this.
    2. The client can potentially tell if it exited in a normal state or in a error condition (noting that we are closing an already closed connection).   Ideally we'd get ever open and every close paired up but admittedly we've been a little bit looser and allowing extra closes.
  13. don't really need getName() (Class is fine)
  14. update comment.  TException?
  15. copyright
  16. not sure why we have made this distinction.  couldn't the multimaster just have an explicit empty?  
  17. similar to my comment in Avro version
  18. WARN if fell through?
  19. this is personal style -- i like @Overrides when implementing a method.
  1. Patrick,
    * Licenses on certain files.
    * DECOMMISSIONED state in avro rpc.
    * I'm still not a fan of having init(xxx) instead of having the parameters plugged into the constructor.  
    Not blockers:
    * Prefer @override on interface implementations
    * return Void.TYPE instead of return null for things that return Void type.
  2. Do we need decomissioned here too?
  3. src/avro/flumeconfig.avdl (Diff revision 4)
    remove line?
  4. src/avro/flumeconfig.avdl (Diff revision 4)
    Needs DECOMISSIONED state.  This currently exists in the thrift version (may have been added after you started on this).
  5. java doc comment for class.
  6. overrides
  7. new constructors? why?
    1. I think these were deleted since I started, and when I rebased it "re-added" them... either way they are redundant and I removed them.
  8. I still don't like not having open. 
  9. Constants?
  10. Why do we need this?  why not put those two parameters into the constructors of the classes that implement them?
  11. I mentioned this before, why aren't the arguments in the constructor, and the variables final?  why not just have a no argument init method after that?
    You wouldn't need the SingleMaster interface then.
  12. no throws IOException?
    1. Thrift's close() doesn't throw an IOException, so we actually have no way of knowing if close doesn't work, for thrift.
  13. what happened to comment?
    1. I put this comment in MasterRPC, better to just @Override right? so if comment changes in the interface it will propagate automatically to this implementation.
  14. thrift for now?
  15. src/java/com/cloudera/flume/master/MasterAdminServer.java (Diff revision 4)
    My version of the getSpecMan().getAllConfigs  already makes a copy of the internal Map, so i think this is uncessary.  This may a be a change from the version you started with.
    for future reference:
    out = new HashMap<..> (orig);
    is simpler than the loop.
    1. Yep, you're right thanks!
  16. prefer overrides for interface implementations.
    1. This class doesn't implement any interfaces. Remember, this is the protocol agnostic class, not the Thrift or Avro implementation.
  17. weird spacing
  18. punctuation before here?
  19. return Void.TYPE instead of null?
    1. This causes compilation errors (knew I had tried this). Avro officially recommends returning null in this case.
  20. return Void.TYPE instead of null?
  21. semantics of close?  Does this block until close? does it just continue to work?
    1. Pretty sure Jetty blocks in a close()... though I think that's okay in this context, right?
  23. interesting, you have DECOMISSIONED here.
  24. oh -- ThriftFlumeConfigData will get agnostified in a future patch right?
    1. yes - didn't do right now because this is in the MasterControl interface. That's next on the chopping block once we get this code in.
  25. I prefer @Override on interface implementations
    1. Avro doesn't put a stop() method in server interface, so this doesn't actually override anything.
    1. Again, this isn't overriding anything.
  26. return Void.TYPE?
    1. Again, needs to be "null", as per Avro best practices.
  27. return Void.TYPE?
Review request changed

Change Summary:

I addressed everything in Jon's review comments. Also one small bugfix. Pull this from my github: 

(DONE) Licenses on certain files.
(DONE) DECOMMISSIONED state in avro rpc.

(DONE) I'm still not a fan of having init(xxx) instead of having the parameters plugged into the constructor.  

Not blockers:
(DONE where applicable) Prefer @override on interface implementations
(null is actually correct - see prior comments) return Void.TYPE instead of return null for things that return Void type.


Revision 5 (+3452 -1720)

Show changes

  1. Responding to Jon's review comments.
  1. lgtm.  
    One nit (no need for review) - is SingleMasterRPC still in the patch or is it removed (and just showing up in review board review?)  If not used, please remove!
    1. Just an artifact of reviewboard. Should be gone if pulled in from github commit listed here. (reminder: 473b428dd1d8e48f2f68)
    2. can you change the status of this to submitted? Thanks!