Properly default idle heartbeat during setter.

[scottf]

Null is a better default.

[scottf]

@claude review

[synadia-claude-reviewer]

Claude finished @scottf's task in 1m 32s —— View job

---

Code Review

Tasks:

• Load tools
• Read CLAUDE.md for repo guidelines (none present)
• Examine the diff
• Review changed file in context
• Post review feedback

---

Summary

The change is in ConsumerConfiguration.java within the idleHeartbeat(Duration) builder setter. When a caller passes a Duration that is zero or negative (i.e., <= DURATION_UNSET_LONG), the field was previously stored as DURATION_UNSET (Duration.ZERO); it is now stored as null.

The fix is correct. Both null and Duration.ZERO are treated identically downstream — JsonUtils.addFieldAsNanos only writes the field when value != null && !value.isZero() && !value.isNegative() (line 272 of JsonUtils.java), so serialization behavior is unchanged. The getter is already annotated @Nullable, so returning null is the correct contract. Using null over a sentinel value of Duration.ZERO is cleaner and more idiomatic Java.

---

Finding

Minor inconsistency: the long overload still uses DURATION_UNSET

ConsumerConfiguration.java, lines 1223–1226:

public Builder idleHeartbeat(long idleHeartbeatMillis) {
    if (idleHeartbeatMillis <= DURATION_UNSET_LONG) {
        this.idleHeartbeat = DURATION_UNSET;   // ← still Duration.ZERO, not null
    }

The Duration overload now consistently stores null for "unset", but the long overload still stores DURATION_UNSET (Duration.ZERO) in the same scenario. Serialization is unaffected (both produce an omitted field), but the builder's internal state differs depending on which overload was used. Any code that null-checks this.idleHeartbeat directly (e.g. a future hasIdleHeartbeat()) would behave differently depending on whether the caller used idleHeartbeat(0L) vs idleHeartbeat(Duration.ZERO).

Fix this →

---

Verdict

The core change is correct and an improvement. The only suggestion is to apply the same null fix to the long overload for consistency. Otherwise, LGTM.
|

[mtmk]

LGTM