Flume
  1. Flume
  2. FLUME-2031

Removal of Unused Imports from Project

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: v1.4.0
    • Fix Version/s: None
    • Component/s: Easy, Technical Debt
    • Labels:
      None

      Description

      Clean up effort to get rid of unused imports throughout the project.

        Issue Links

          Activity

          Hide
          Israel Ekpo added a comment -
          Show
          Israel Ekpo added a comment - Review is ready https://reviews.apache.org/r/10830/
          Hide
          Hari Shreedharan added a comment -

          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

          Show
          Hari Shreedharan added a comment - 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
          Hide
          Hari Shreedharan added a comment -

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

          Show
          Hari Shreedharan added a comment - ScribeSource.java is not thrift generated - sorry about that.
          Hide
          Israel Ekpo added a comment -

          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?

          Show
          Israel Ekpo added a comment - 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?
          Hide
          Israel Ekpo added a comment -

          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?

          Show
          Israel Ekpo added a comment - 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?
          Hide
          Hari Shreedharan added a comment -

          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.

          Show
          Hari Shreedharan added a comment - 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.
          Hide
          Mike Percy added a comment -

          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.

          Show
          Mike Percy added a comment - 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.
          Hide
          Israel Ekpo added a comment -

          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?

          Show
          Israel Ekpo added a comment - 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?
          Hide
          Mike Percy added a comment -

          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.

          Show
          Mike Percy added a comment - 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.

            People

            • Assignee:
              Israel Ekpo
              Reporter:
              Israel Ekpo
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Due:
                Created:
                Updated:

                Development