Review is ready

https://reviews.apache.org/r/10830/

Israel,

Looks good. The following files are thrift generated and they will simply get overwritten when you run mvn install -PcompileThrift:

flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/EventStatus.java
flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/Priority.java
flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEvent.java
flume-ng-legacy-sources/flume-thrift-source/src/main/java/com/cloudera/flume/handlers/thrift/ThriftFlumeEventServer.java
flume-ng-sdk/src/main/java/org/apache/flume/thrift/Status.java
flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftFlumeEvent.java
flume-ng-sdk/src/main/java/org/apache/flume/thrift/ThriftSourceProtocol.java
flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/LogEntry.java
flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ResultCode.java
flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/Scribe.java
flume-ng-sources/flume-scribe-source/src/main/java/org/apache/flume/source/scribe/ScribeSource.java

ScribeSource.java is not thrift generated - sorry about that.

Thanks for letting me know.

I will update the patch and exclude those.

Besides the Thrift classes, are there any other identifiers that are auto generated?

According to Brock Noland, we do check in generated source code for Thrift/Protobuf/Avro

I will add those to my exclusion list.

Could you confirm this?

Yes, we do check in the generated code for thrift and protobuf (we don't commit the avro code I think - that is generated during build by the avro maven plugin), but only the thrift generated sources are in the list of classes in your patch.

I'm not a fan of doing big cleanups like this. I feel like it generates a lot of noise for little concrete benefit. Plus, we are reordering imports for no benefit since every IDE has different ordering rules.

I did not reorder the imports manually, it was done automatically be Eclipse (Ctrl + Shift + O)

I can take out the unused imports one by one next time.

So would you prefer if it was done on a file-by-file or one-package-at-a-time basis?

Unused imports should be removed when updating the file for some other reason, like fixing a bug or adding a new feature. A mass update like this just adds regression risk for little benefit in my opinion. I feel that stuff like this isn't something we should spend much time worrying about or actively fix.

+1. We will optimize imports when we commit changes to a file as part of another issue. I don't think we should be actively working on this. Closing.