kfaraz commented on code in PR #18480:
URL: https://github.com/apache/druid/pull/18480#discussion_r2328497511
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/autoscaler/LagBasedAutoScalerConfig.java:
##########
@@ -212,6 +218,15 @@ public AggregateFunction getLagAggregate()
return lagAggregate;
}
+ @Override
+ @JsonProperty
+ @Nullable
+ @JsonInclude(JsonInclude.Include.NON_NULL)
Review Comment:
> Of course. I think I was just trying to point out that we should have a
standard for leaving null (but optional) fields in a config, versus keeping
them out.
Yeah, that would be nice. I think this could be a config on the object
mapper itself.
> I think it does.
Oh, I must be missing something then. The only fields that would have ever
have a null value are nullable (i.e. non required) fields. Required fields
should always be validated in the constructor or assigned a default value if
null. So the serialized JSON will always have a non-null value for required
fields. A malformed input should fail during deserializing itself.
That said, it is really fine either way. It is only a style choice. 🙂
And it seems that in the entirety of Druid code, this annotation is never
used on the class level.
So we can just stick to that for the time being.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
