KAFKA-7011 - Remove ResourceNameType field from Java Resource class. #5160
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cmccabe
reviewed
Jun 7, 2018
| * @param name non-null resource name, which can be the {@link #WILDCARD_RESOURCE}. | ||
| * @param nameType non-null, specific, resource name type, which controls how the pattern will match resource names. | ||
| */ | ||
| public ResourcePattern(ResourceType resourceType, String name, ResourceNameType nameType) { |
There was a problem hiding this comment.
I really like the fact that we are separating Resources from ResourcePatterns! Great job.
cmccabe
reviewed
Jun 7, 2018
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * Represents a pattern that can match a {@link org.apache.kafka.common.resource.Resource}. |
There was a problem hiding this comment.
Perhaps something like "Represents a pattern that is used by ACLs to match zero or more Resources"
There was a problem hiding this comment.
Currently used by ACLs only, but was trying to keep it ACL free in the description. Still want me to change?
cmccabe
reviewed
Jun 7, 2018
| import static org.apache.kafka.common.resource.ResourcePattern.WILDCARD_RESOURCE; | ||
|
|
||
| /** | ||
| * Represents a filter that can match {@link ResourcePatternFilter}. |
There was a problem hiding this comment.
s/ResourcePatternFilter/ResourcePattern/
|
+1. LGTM. I really don't have anything to add except for the comment suggestion. |
3 tasks
junrao
approved these changes
Jun 7, 2018
junrao
pushed a commit
that referenced
this pull request
Jun 7, 2018
…5160) The initial PR for KIP-290 #5117 added a `ResourceNameType` field to the Java and Scala `Resource` classes to introduce the concept of Prefixed ACLS. This does not make a lot of sense as these classes are meant to represent cluster resources, which would not have a concept of 'name type'. This work has not been released yet, so we have time to change it. This PR looks to refactor the code to remove the name type field from the Java `Resource` class. (The Scala one will age out once KIP-290 is done, and removing it would involve changes to the `Authorizer` interface, so this class was not touched). This is achieved by replacing the use of `Resource` with `ResourcePattern` and `ResourceFilter` with `ResourceFilterPattern`. A `ResourcePattern` is a combination of resource type, name and name type, where each field needs to be defined. A `ResourcePatternFilter` is used to select patterns during describe and delete operations. The adminClient uses `AclBinding` and `AclBindingFilter`. These types have been switched over to use the new pattern types. The AclCommands class, used by Kafka-acls.sh, has been converted to use the new pattern types. The result is that the original `Resource` and `ResourceFilter` classes are not really used anywhere, except deprecated methods. However, the `Resource` class will be used if/when KIP-50 is done. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
ying-zheng
pushed a commit
to ying-zheng/kafka
that referenced
this pull request
Jul 6, 2018
…pache#5160) The initial PR for KIP-290 apache#5117 added a `ResourceNameType` field to the Java and Scala `Resource` classes to introduce the concept of Prefixed ACLS. This does not make a lot of sense as these classes are meant to represent cluster resources, which would not have a concept of 'name type'. This work has not been released yet, so we have time to change it. This PR looks to refactor the code to remove the name type field from the Java `Resource` class. (The Scala one will age out once KIP-290 is done, and removing it would involve changes to the `Authorizer` interface, so this class was not touched). This is achieved by replacing the use of `Resource` with `ResourcePattern` and `ResourceFilter` with `ResourceFilterPattern`. A `ResourcePattern` is a combination of resource type, name and name type, where each field needs to be defined. A `ResourcePatternFilter` is used to select patterns during describe and delete operations. The adminClient uses `AclBinding` and `AclBindingFilter`. These types have been switched over to use the new pattern types. The AclCommands class, used by Kafka-acls.sh, has been converted to use the new pattern types. The result is that the original `Resource` and `ResourceFilter` classes are not really used anywhere, except deprecated methods. However, the `Resource` class will be used if/when KIP-50 is done. Reviewers: Colin Patrick McCabe <colin@cmccabe.xyz>, Jun Rao <junrao@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix for KAFKA-7011.
The initial PR for KIP-290 #5117 added a
ResourceNameTypefield to the Java and ScalaResourceclasses to introduce the concept of Prefixed ACLS. This does not make a lot of sense as these classes are meant to represent cluster resources, which would not have a concept of 'name type'. This work has not been released yet, so we have time to change it.This PR looks to refactor the code to remove the name type field from the Java
Resourceclass. (The Scala one will age out once KIP-290 is done, and removing it would involve changes to theAuthorizerinterface, so this class was not touched).This is achieved by replacing the use of
ResourcewithResourcePatternandResourceFilterwithResourceFilterPattern. AResourcePatternis a combination of resource type, name and name type, where each field needs to be defined. AResourcePatternFilteris used to select patterns during describe and delete operations.The adminClient uses
AclBindingandAclBindingFilter. These types have been switched over to use the new pattern types.The AclCommands class, used by Kafka-acls.sh, has been converted to use the new pattern types.
The result is that the original
ResourceandResourceFilterclasses are not really used anywhere, except deprecated methods. However, theResourceclass will be used if/when KIP-50 is done.cc @cmccabe, @junrao
This PR will need cherry picking onto the 2.0 branch.
Committer Checklist (excluded from commit message)