Jackie-Jiang commented on code in PR #19529:
URL: https://github.com/apache/pinot/pull/19529#discussion_r4498724358
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/SegmentConsumerInfo.java:
##########
@@ -31,23 +33,41 @@ public class SegmentConsumerInfo {
private final long _lastConsumedTimestamp;
private final Map<String, String> _partitionToOffsetMap;
private final PartitionOffsetInfo _partitionOffsetInfo;
+ // FQCN of the StreamMessageDecoder instantiated on the consumer, or null if
decoder init has not completed.
+ @Nullable
+ private final String _decoderClassName;
+
+ /// Kept for source and binary compatibility with callers that build this
DTO without the decoder class name. Not a
+ /// Jackson creator; deserialization goes through the `@JsonCreator`
constructor below.
+ public SegmentConsumerInfo(String segmentName, String consumerState, long
lastConsumedTimestamp,
+ Map<String, String> partitionToOffsetMap, PartitionOffsetInfo
partitionOffsetInfo) {
+ this(segmentName, consumerState, lastConsumedTimestamp,
partitionToOffsetMap, partitionOffsetInfo, null);
+ }
+ @JsonCreator
public SegmentConsumerInfo(@JsonProperty("segmentName") String segmentName,
@JsonProperty("consumerState") String consumerState,
@JsonProperty("lastConsumedTimestamp") long lastConsumedTimestamp,
@JsonProperty("partitionToOffsetMap") Map<String, String>
partitionToOffsetMap,
- @JsonProperty("partitionOffsetInfo") PartitionOffsetInfo
partitionOffsetInfo) {
+ @JsonProperty("partitionOffsetInfo") PartitionOffsetInfo
partitionOffsetInfo,
+ @JsonProperty("decoderClassName") @Nullable String decoderClassName) {
_segmentName = segmentName;
_consumerState = consumerState;
_lastConsumedTimestamp = lastConsumedTimestamp;
_partitionToOffsetMap = partitionToOffsetMap;
_partitionOffsetInfo = partitionOffsetInfo;
+ _decoderClassName = decoderClassName;
}
public String getSegmentName() {
return _segmentName;
}
+ @Nullable
+ public String getDecoderClassName() {
Review Comment:
Please move `getDecoderClassName()` after `getPartitionOffsetInfo()` so the
getter order matches the field declarations and constructor parameters.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/ConsumingSegmentInfoReader.java:
##########
@@ -210,18 +212,31 @@ static public class ConsumingSegmentInfo {
public Map<String, String> _partitionToOffsetMap;
@JsonProperty("partitionOffsetInfo")
public PartitionOffsetInfo _partitionOffsetInfo;
+ @JsonProperty("decoderClassName")
+ @Nullable
+ public String _decoderClassName;
+ /// Kept for source and binary compatibility with callers that build this
DTO without the decoder class name. Not a
+ /// Jackson creator; deserialization goes through the `@JsonCreator`
constructor below.
+ public ConsumingSegmentInfo(String serverName, String consumerState, long
lastConsumedTimestamp,
+ Map<String, String> partitionToOffsetMap, PartitionOffsetInfo
partitionOffsetInfo) {
+ this(serverName, consumerState, lastConsumedTimestamp,
partitionToOffsetMap, partitionOffsetInfo, null);
+ }
Review Comment:
We do not need to preserve binary compatibility for `ConsumingSegmentInfo`.
Please remove this legacy constructor, its compatibility comment, and the
corresponding legacy-constructor test assertion. Keep the six-argument
`@JsonCreator` constructor and the missing-field JSON test for rolling upgrades.
##########
pinot-controller/src/main/java/org/apache/pinot/controller/util/ConsumingSegmentInfoReader.java:
##########
@@ -210,18 +212,31 @@ static public class ConsumingSegmentInfo {
public Map<String, String> _partitionToOffsetMap;
@JsonProperty("partitionOffsetInfo")
public PartitionOffsetInfo _partitionOffsetInfo;
+ @JsonProperty("decoderClassName")
+ @Nullable
+ public String _decoderClassName;
Review Comment:
Please collapse these consecutive empty lines to a single empty line.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeSegmentDataManager.java:
##########
@@ -1503,6 +1503,15 @@ private void releaseConsumerSemaphore() {
}
}
+ /// @return the FQCN of the [StreamMessageDecoder] actually instantiated on
this consumer (the source of truth for
+ /// what is decoding this segment), or `null` if decoder init has
not completed.
+ @Nullable
+ public String getDecoderClassName() {
+ StreamDataDecoder decoder = _streamDataDecoder;
+ StreamMessageDecoder valueDecoder = decoder == null ? null :
decoder.getValueDecoder();
+ return valueDecoder == null ? null : valueDecoder.getClass().getName();
Review Comment:
`_streamDataDecoder` is initialized during construction, so its null guard
and local variable are unnecessary. We also usually put `!= null` in the check:
```suggestion
StreamMessageDecoder<?> valueDecoder =
_streamDataDecoder.getValueDecoder();
return valueDecoder != null ? valueDecoder.getClass().getName() : null;
```
--
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]
