-
Notifications
You must be signed in to change notification settings - Fork 13.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-7005: Remove duplicate resource class. #5184
Conversation
@@ -33,7 +34,7 @@ import org.apache.kafka.common.protocol.types.Struct | |||
import org.apache.kafka.common.resource.ResourceNameType | |||
import org.apache.kafka.common.record._ | |||
import org.apache.kafka.common.requests.CreateAclsRequest.AclCreation | |||
import org.apache.kafka.common.requests.{Resource => RResource, ResourceType => RResourceType, _} | |||
import org.apache.kafka.common.requests._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really be using star imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my defence, I've only removed imports here, not added them.
This is also a test class that pulls in a LOT of classes from that package - it would be a long long list!
import org.apache.kafka.common.errors._ | ||
import org.apache.kafka.common.internals.Topic.GROUP_METADATA_TOPIC_NAME | ||
import org.apache.kafka.common.network.ListenerName | ||
import org.apache.kafka.common.protocol.{ApiKeys, Errors} | ||
import org.apache.kafka.common.record.{CompressionType, MemoryRecords, Records, SimpleRecord} | ||
import org.apache.kafka.common.requests.CreateAclsRequest.AclCreation | ||
import org.apache.kafka.common.requests.CreateTopicsRequest.TopicDetails | ||
import org.apache.kafka.common.requests.{Resource => RResource, ResourceType => RResourceType, _} | ||
import org.apache.kafka.common.requests._ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really be using star imports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above.
LGTM, after question about star imports is addressed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@big-andy-coates : Thanks for the patch. LGTM
This is a follow-on change requested as part of the initial PR for KIP-290 #5117. @cmccabe requested that the `resource.Resource` class be factored out in favour of `ConfigResource` to avoid confusion between all the `Resource` implementations. Colin Patrick McCabe <[email protected]>, Jun Rao <[email protected]>
This is a follow-on change requested as part of the initial PR for KIP-290 apache#5117. @cmccabe requested that the `resource.Resource` class be factored out in favour of `ConfigResource` to avoid confusion between all the `Resource` implementations. Colin Patrick McCabe <[email protected]>, Jun Rao <[email protected]>
Fix for KAFKA-7005.
This is a follow-on change requested as part of the initial PR for KIP-290 #5117. @cmccabe requested that the
resource.Resource
class be factored out in favour ofConfigResource
to avoid confusion between all theResource
implementations.cc @cmccabe, @junrao
Committer Checklist (excluded from commit message)