You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/10/04 16:31:29 UTC
[GitHub] [kafka] Fleshgrinder opened a new pull request #9370: KAFA-9234: Added Nullability Annotations to Admin
Fleshgrinder opened a new pull request #9370:
URL: https://github.com/apache/kafka/pull/9370
This introduces a new **compile-only** dependency on [JetBrains Java Annotations](https://github.com/JetBrains/java-annotations). I decided to go for this particular dependency because it does not introduce any visible change to our users, nor does it introduce any additional overhead to the build process. If we get an annotation wrong, nothing bad happens because it is totally up to the code to actually validate things. (This also means that no KIP is required.)
One downside (some might say upside) is that absolutely everything must be annotated. I personally think this is a good thing because it forces people to actually think about `null`, same is the case in Kotlin where I also have to define whether `null` is permitted or not absolutely everywhere. Kotlin is also the main reason why I am interested in this, and also why I am interested in having this everywhere.
I was not able to test all possible NPE situations because some require the call to actually be executed. For those where I was able to proof that `null` is invalid we could think about adding `Objects.requireNonNull` guards right at the beginning. For all others it simple, if we proof it we can add the guards too. This might be wasteful for collections (especially if those collections contain collections). A possible way out here is to use `assert` and life with the fact that some NPEs are thrown much later and will make it hard to find the source. This, sadly, is not Kotlin, so there is no easy way for this.
## Bugfix
This PR also contains a bug fix. `Admin.describeUserScramCredentials()` (without any arguments) was passing `null` to `Admin.describeUserScramCredentials(List<String>)` which is not permitted and leads to an NPE. Maybe there is another ticket for this, or nobody ever called this method (which would raise the question why it exists). In any event, the documentation of both `Admin.describeUserScramCredentials(List<String>)` and `Admin.describeUserScramCredentials(List<String>, DescribeUserScramCredentialsOptions)` say in their doc that `users` may be `null` and that all users’ credentials are fetched if that is the case (same for an empty list), however, in `KafkaAdminClient` we have an unconditional call on `users.stream()` that leads to said NPE. I removed the mention of `null`, not sure what the stance is regarding doc changes and their relation to breaking changes. This effectively just aligns the doc with the real code and the behavior does not change, but maybe we actually w
ant the `null` support here, in that case we would need to change the code. Let me know what you think.
I think that it would be best to remove this implicit behavior (which effectively is just a doc change) and add an explicit method to the interface that is called `describeAllUserScramCredentials`.
PS: I am willing to continue this endeavour if it is decided to accept this PR (also if we decide to opt for another library that provides the ability to add null checks, e.g. Checker Framework, Lombok), simply because the experience in Kotlin with Kafka and the missing nullability information is horrid.
PPS: The JetBrains annotations contain additional annotations like `@Unmodifiable`, `@ApiStatus.Internal`, `@RegExp`, or `@Language` that ease development work tremendously. I think it would be more than worthwhile to start using them as well if we decide to go with these annotations.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on a change in pull request #9370: KAFKA-9234: Added Nullability Annotations to Admin
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #9370:
URL: https://github.com/apache/kafka/pull/9370#discussion_r499647441
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -4160,15 +4231,18 @@ void handleFailure(Throwable throwable) {
}
@Override
- public DescribeUserScramCredentialsResult describeUserScramCredentials(List<String> users, DescribeUserScramCredentialsOptions options) {
+ public @NotNull DescribeUserScramCredentialsResult describeUserScramCredentials(
+ final @Nullable List<@NotNull String> users,
+ final @NotNull DescribeUserScramCredentialsOptions options
+ ) {
final KafkaFutureImpl<DescribeUserScramCredentialsResponseData> dataFuture = new KafkaFutureImpl<>();
final long now = time.milliseconds();
Call call = new Call("describeUserScramCredentials", calcDeadlineMs(now, options.timeoutMs()),
new LeastLoadedNodeProvider()) {
@Override
public DescribeUserScramCredentialsRequest.Builder createRequest(int timeoutMs) {
return new DescribeUserScramCredentialsRequest.Builder(
- new DescribeUserScramCredentialsRequestData().setUsers(users.stream().map(user ->
+ new DescribeUserScramCredentialsRequestData().setUsers(users == null ? Collections.emptyList() : users.stream().map(user ->
Review comment:
I’m thinking it would be best to fix this bug in a separate “MINOR: fix potential NPE...” PR since this PR may or may not get merged. Do you agree? If so, either I can do it, or feel free to do it — I’ll go with whichever you wish.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on a change in pull request #9370: KAFKA-9234: Added Nullability Annotations to Admin
Posted by GitBox <gi...@apache.org>.
rondagostino commented on a change in pull request #9370:
URL: https://github.com/apache/kafka/pull/9370#discussion_r499595845
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -4160,15 +4231,18 @@ void handleFailure(Throwable throwable) {
}
@Override
- public DescribeUserScramCredentialsResult describeUserScramCredentials(List<String> users, DescribeUserScramCredentialsOptions options) {
+ public @NotNull DescribeUserScramCredentialsResult describeUserScramCredentials(
+ final @Nullable List<@NotNull String> users,
+ final @NotNull DescribeUserScramCredentialsOptions options
+ ) {
final KafkaFutureImpl<DescribeUserScramCredentialsResponseData> dataFuture = new KafkaFutureImpl<>();
final long now = time.milliseconds();
Call call = new Call("describeUserScramCredentials", calcDeadlineMs(now, options.timeoutMs()),
new LeastLoadedNodeProvider()) {
@Override
public DescribeUserScramCredentialsRequest.Builder createRequest(int timeoutMs) {
return new DescribeUserScramCredentialsRequest.Builder(
- new DescribeUserScramCredentialsRequestData().setUsers(users.stream().map(user ->
+ new DescribeUserScramCredentialsRequestData().setUsers(users == null ? Collections.emptyList() : users.stream().map(user ->
Review comment:
Not 100% sure the parens are needed, but either they are or it increases clarity/decreases confusion to add them.
```suggestion
new DescribeUserScramCredentialsRequestData().setUsers((users == null ? Collections.emptyList() : users).stream().map(user ->
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] rondagostino commented on pull request #9370: KAFA-9234: Added Nullability Annotations to Admin
Posted by GitBox <gi...@apache.org>.
rondagostino commented on pull request #9370:
URL: https://github.com/apache/kafka/pull/9370#issuecomment-703313051
@Fleshgrinder Thanks for the PR. I'll let others comment on the main gist of it (`null` has probably wasted more than a billion dollars, so I'm generally all for mitigating the damage to the extent possible).
Regarding the bug you found, I believe that since KIP-554 stated the behavior publicly the behavior has to stay that way unless there is another KIP to change it. One approach is to add a test that exposes the issue and commit that test and the corresponding fix. The issue can be exposed by eliminating the `usersRequestedList` parameter at https://github.com/apache/kafka/blob/trunk/clients/src/test/java/org/apache/kafka/clients/admin/KafkaAdminClientTest.java#L4528. I would be glad to open a PR if that is ultimately how we decide we want to go, or feel free to do what you want/need here based on the discussion -- either way is fine with me.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9370: KAFKA-9234: Added Nullability Annotations to Admin
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9370:
URL: https://github.com/apache/kafka/pull/9370#discussion_r499654983
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -4160,15 +4231,18 @@ void handleFailure(Throwable throwable) {
}
@Override
- public DescribeUserScramCredentialsResult describeUserScramCredentials(List<String> users, DescribeUserScramCredentialsOptions options) {
+ public @NotNull DescribeUserScramCredentialsResult describeUserScramCredentials(
+ final @Nullable List<@NotNull String> users,
+ final @NotNull DescribeUserScramCredentialsOptions options
+ ) {
final KafkaFutureImpl<DescribeUserScramCredentialsResponseData> dataFuture = new KafkaFutureImpl<>();
final long now = time.milliseconds();
Call call = new Call("describeUserScramCredentials", calcDeadlineMs(now, options.timeoutMs()),
new LeastLoadedNodeProvider()) {
@Override
public DescribeUserScramCredentialsRequest.Builder createRequest(int timeoutMs) {
return new DescribeUserScramCredentialsRequest.Builder(
- new DescribeUserScramCredentialsRequestData().setUsers(users.stream().map(user ->
+ new DescribeUserScramCredentialsRequestData().setUsers(users == null ? Collections.emptyList() : users.stream().map(user ->
Review comment:
Happy to help, PR incoming. 😊
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9370: KAFKA-9234: Added Nullability Annotations to Admin
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9370:
URL: https://github.com/apache/kafka/pull/9370#discussion_r499643570
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -4160,15 +4231,18 @@ void handleFailure(Throwable throwable) {
}
@Override
- public DescribeUserScramCredentialsResult describeUserScramCredentials(List<String> users, DescribeUserScramCredentialsOptions options) {
+ public @NotNull DescribeUserScramCredentialsResult describeUserScramCredentials(
+ final @Nullable List<@NotNull String> users,
+ final @NotNull DescribeUserScramCredentialsOptions options
+ ) {
final KafkaFutureImpl<DescribeUserScramCredentialsResponseData> dataFuture = new KafkaFutureImpl<>();
final long now = time.milliseconds();
Call call = new Call("describeUserScramCredentials", calcDeadlineMs(now, options.timeoutMs()),
new LeastLoadedNodeProvider()) {
@Override
public DescribeUserScramCredentialsRequest.Builder createRequest(int timeoutMs) {
return new DescribeUserScramCredentialsRequest.Builder(
- new DescribeUserScramCredentialsRequestData().setUsers(users.stream().map(user ->
+ new DescribeUserScramCredentialsRequestData().setUsers(users == null ? Collections.emptyList() : users.stream().map(user ->
Review comment:
This way we would also run the empty map through _stream_, which is not required. I tried to make the change with as little impact as possible but if I were to write it then I would do it as follows:
```java
Call call = new Call("describeUserScramCredentials", calcDeadlineMs(now, options.timeoutMs()), new LeastLoadedNodeProvider()) {
@Override
public DescribeUserScramCredentialsRequest.Builder createRequest(final int timeoutMs) {
final DescribeUserScramCredentialsRequestData requestData = new DescribeUserScramCredentialsRequestData();
if (users != null) {
final List<UserName> userNames = new ArrayList<>(users.size());
for (final String user : users) {
userNames.add(new UserName().setName(user));
}
requestData.setUsers(userNames);
}
return new DescribeUserScramCredentialsRequest.Builder(requestData);
}
```
This is more code, yes, and it is not using _stream_ anymore but it is both clearer, faster, and allocates less.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Fleshgrinder commented on a change in pull request #9370: KAFKA-9234: Added Nullability Annotations to Admin
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on a change in pull request #9370:
URL: https://github.com/apache/kafka/pull/9370#discussion_r499643570
##########
File path: clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java
##########
@@ -4160,15 +4231,18 @@ void handleFailure(Throwable throwable) {
}
@Override
- public DescribeUserScramCredentialsResult describeUserScramCredentials(List<String> users, DescribeUserScramCredentialsOptions options) {
+ public @NotNull DescribeUserScramCredentialsResult describeUserScramCredentials(
+ final @Nullable List<@NotNull String> users,
+ final @NotNull DescribeUserScramCredentialsOptions options
+ ) {
final KafkaFutureImpl<DescribeUserScramCredentialsResponseData> dataFuture = new KafkaFutureImpl<>();
final long now = time.milliseconds();
Call call = new Call("describeUserScramCredentials", calcDeadlineMs(now, options.timeoutMs()),
new LeastLoadedNodeProvider()) {
@Override
public DescribeUserScramCredentialsRequest.Builder createRequest(int timeoutMs) {
return new DescribeUserScramCredentialsRequest.Builder(
- new DescribeUserScramCredentialsRequestData().setUsers(users.stream().map(user ->
+ new DescribeUserScramCredentialsRequestData().setUsers(users == null ? Collections.emptyList() : users.stream().map(user ->
Review comment:
This way we would also run the empty map through _stream_, which is not required. I tried to make the change with as little impact as possible but if I were to write it then I would do it as follows:
```java
Call call = new Call("describeUserScramCredentials", calcDeadlineMs(now, options.timeoutMs()), new LeastLoadedNodeProvider()) {
@Override
public DescribeUserScramCredentialsRequest.Builder createRequest(final int timeoutMs) {
final DescribeUserScramCredentialsRequestData requestData = new DescribeUserScramCredentialsRequestData();
if (users != null && !users.isEmpty()) {
final List<UserName> userNames = new ArrayList<>(users.size());
for (final String user : users) {
userNames.add(new UserName().setName(user));
}
requestData.setUsers(userNames);
}
return new DescribeUserScramCredentialsRequest.Builder(requestData);
}
```
This is more code, yes, and it is not using _stream_ anymore but it is both clearer, faster, and allocates less.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] Fleshgrinder commented on pull request #9370: KAFA-9234: Added Nullability Annotations to Admin
Posted by GitBox <gi...@apache.org>.
Fleshgrinder commented on pull request #9370:
URL: https://github.com/apache/kafka/pull/9370#issuecomment-703439333
Hey @rondagostino 😊 we can equally well fix it in the other direction and allow `null`. Let me update the PR and revert the doc changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org