KAFKA-3009 : Disallow star imports

[manasvigupta]

Summary of code changes

1. Added a new Checkstyle rule to flag any code using star imports
2. Fixed ALL existing code violations using star imports

Testing

Local build was successful
ALL JUnits ran successfully on local.

@ewencp - Request you to please review changes. Thank you !

I state that the contribution is my original work and I license the work to the project under the project's open source license.

[ewencp]

Is this line doing anything? From the checkstyle docs it looks like it's the default? Would a single <module name="AvoidStarImport" /> line here be enough?

[manasvigupta]

Yes, a single line <module name="AvoidStarImport" /> would suffice. I just tested this on my local and it works. I will re-issue pull request with this update.

Thanks again for your feedback!

[ewencp]

@manasvigupta Minor question, but this patch looks good.

Another question though -- it looks like there's still at least one case of a star import:

$ git grep ^import | grep \*
core/src/main/scala/kafka/tools/KafkaMigrationTool.java:import joptsimple.*;

But it isn't being caught. I'm guessing this because it is actually under the scala directory (despite being a java file) and checkstyle simply isn't checking files under that directory? It does seem like other star imports are being caught if I add them.

[granthenke]

@ewencp Right now checkstyle is not configured for the core module in the build. I had a major gradle refactor that added it, but closed it to break the patch into smaller changes. Its being tracked under KAFKA-2787. I can make some subtasks and a few PRs this week with those changes.

[manasvigupta]

@ewencp - I tried to add checktyle to core module build. and found numerous checkstyle errors for existing java code. For reference, pls see checkstyle report attached to jira ticket - https://issues.apache.org/jira/secure/attachment/12778864/main.xml

Of these errors, this one seems most significant - "Import control file does not handle this package". Essentially, we need to define a new import control file (or update existing one) for package - "kafka" under core module.

So, what would you like me to do here?

My suggestion is -

1. fix the original issue for non-core modules. (I can raise new pull request incorporating your review comment).
2. create separate jira ticket to apply checkstyle rules to core module and fix violations (or add subtasks to existing ticket KAFKA-2787 created by @granthenke ).

Thoughts?

[ewencp]

@manasvigupta That sounds good -- I'm all for incremental progress. I just grepped through the code to sanity check that we had caught all the star imports and wasn't immediately sure why that one was still there.

[granthenke]

@ewencp @manasvigupta I have all the checkstyle changes for core in the closed (not pulled in or reviewed) PR here: #477

I had to add separate import control file for core since it doesn't follow the o.a.k structure. That can be seen here: https://github.com/apache/kafka/pull/477/files#diff-c22a094be242401320f23d891dad1508

I can add a subtask and send a pr with that piece of work today if you like.

[ewencp]

@granthenke That would be excellent.

@manasvigupta There was just the minor tweak to the XML file left -- if you push an update to fix that, I'm happy to merge this.

[granthenke]

Created KAFKA-3020 to track the change for core.

[manasvigupta]

@ewencp Yes, I will push update for it.

[granthenke]

@ewencp Created related pull request #703

[manasvigupta]

@ewencp Pushed update to this PR.