search

strimzi / metrics-reporter

Prometheus Metrics Reporter for Apache Kafka server and client components
Apache License 2.0
5 stars 9 forks source link

Fix spotbugs errors #31

Closed mimaison closed 3 months ago

mimaison commented 3 months ago

Put default values wherever possible, otherwise assert not null before using fields not initialized in constructors.

Frawless commented 3 months ago

I'm not sure how much we want to keep this in sync with the rest of the Strimzi code ... in Strimzi, we would normally:

* Keep the annotation to silent the warning in places where not initializing the field in the reasonable choice

* I guess we would normally just use `if` instead of `Objects.requireNonNull`

But I'm not sure how much we want to keep it in sync given it is a different project mostly with different people contributing. So maybe we can just go with it.

Any thoughts @strimzi/maintainers?

I don't have strong opinion, I am fine with the both options.

Just wonder if depedency for annotations should be removed as it might get useful in the future...on the other hand guess it cause some troubles with unused dependency check ?

im-konge commented 3 months ago

I guess we would normally just use if instead of Objects.requireNonNull

Yeah I think that using the if would be maybe better, because this will anyway produce NPE when the collector will not be initialized, or? But will there be a situation when it will not be configured? Or instead of NPE, should it throw some reasonable exception with some information?

Keep the annotation to silent the warning in places where not initializing the field in the reasonable choice

Yeah for example for the httpServer it looks reasonable to have the annotation there, as it will be anyway configured as part of the configure method.

I'm not that familiar with the code, but how much is this related to #20 ?

mimaison commented 3 months ago

Just wonder if depedency for annotations should be removed as it might get useful in the future

I'm not sure it makes sense keeping an unused dependency just in case we need it in the future. If we need it later, it's easy to add it back.

mimaison commented 3 months ago

Ok, I'll follow what's done in other projects and keep the annotations. I pushed an updated and I added some details to each to explain why it's fine to suppress the warning.