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: us...@infra.apache.org
