Jackie-Jiang commented on code in PR #18853:
URL: https://github.com/apache/pinot/pull/18853#discussion_r3496498517
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsBitmapResponse.java:
##########
@@ -18,24 +18,40 @@
*/
package org.apache.pinot.common.restlet.resources;
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
+import javax.annotation.Nullable;
import org.apache.pinot.common.utils.ServiceStatus;
+@JsonIgnoreProperties(ignoreUnknown = true)
public class ValidDocIdsBitmapResponse {
private final String _segmentName;
private final String _segmentCrc;
+ // Server's data CRC (forward index + dictionary checksum); null when the
server doesn't report it.
+ @Nullable
+ private final String _segmentDataCrc;
private final ValidDocIdsType _validDocIdsType;
private final byte[] _bitmap;
private final String _instanceId;
private final ServiceStatus.Status _serverStatus;
+ public ValidDocIdsBitmapResponse(String segmentName, String crc,
ValidDocIdsType validDocIdsType, byte[] bitmap,
Review Comment:
(minor) Let's not add this constructor. We don't need backward compatibility
for this class
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsMetadataInfo.java:
##########
@@ -30,24 +33,37 @@ public class ValidDocIdsMetadataInfo {
private final long _totalInvalidDocs;
private final long _totalDocs;
private final String _segmentCrc;
+ // Server's data CRC (forward index + dictionary checksum); null when the
server doesn't report it.
+ @Nullable
+ private final String _segmentDataCrc;
private final ValidDocIdsType _validDocIdsType;
private final long _segmentSizeInBytes;
private final long _segmentCreationTimeMillis;
private final String _instanceId;
private final ServiceStatus.Status _serverStatus;
+ public ValidDocIdsMetadataInfo(String segmentName, long totalValidDocs, long
totalInvalidDocs, long totalDocs,
Review Comment:
(minor) Let's not add this constructor. We don't need backward compatibility
for this class
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsBitmapResponse.java:
##########
@@ -18,24 +18,40 @@
*/
package org.apache.pinot.common.restlet.resources;
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
+import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
+import javax.annotation.Nullable;
import org.apache.pinot.common.utils.ServiceStatus;
+@JsonIgnoreProperties(ignoreUnknown = true)
public class ValidDocIdsBitmapResponse {
private final String _segmentName;
private final String _segmentCrc;
+ // Server's data CRC (forward index + dictionary checksum); null when the
server doesn't report it.
+ @Nullable
+ private final String _segmentDataCrc;
private final ValidDocIdsType _validDocIdsType;
private final byte[] _bitmap;
private final String _instanceId;
private final ServiceStatus.Status _serverStatus;
+ public ValidDocIdsBitmapResponse(String segmentName, String crc,
ValidDocIdsType validDocIdsType, byte[] bitmap,
+ String instanceId, ServiceStatus.Status serverStatus) {
+ this(segmentName, crc, validDocIdsType, bitmap, instanceId, serverStatus,
null);
+ }
+
+ @JsonCreator
public ValidDocIdsBitmapResponse(@JsonProperty("segmentName") String
segmentName,
@JsonProperty("segmentCrc") String crc, @JsonProperty("validDocIdsType")
ValidDocIdsType validDocIdsType,
@JsonProperty("bitmap") byte[] bitmap, @JsonProperty("instanceId")
String instanceId,
- @JsonProperty("serverStatus") ServiceStatus.Status serverStatus) {
+ @JsonProperty("serverStatus") ServiceStatus.Status serverStatus,
+ @JsonProperty("segmentDataCrc") @Nullable String segmentDataCrc) {
Review Comment:
(minor) Keep the argument order consistent with declaration.
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsBitmapResponse.java:
##########
@@ -65,4 +81,11 @@ public String getInstanceId() {
public ServiceStatus.Status getServerStatus() {
return _serverStatus;
}
+
+ /// Server's data CRC, or null if not reported. Omitted from the payload
when null.
+ @JsonInclude(JsonInclude.Include.NON_NULL)
+ @Nullable
+ public String getSegmentDataCrc() {
Review Comment:
(minor) Keep the getter order consistent with declaration.
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsMetadataInfo.java:
##########
@@ -30,24 +33,37 @@ public class ValidDocIdsMetadataInfo {
private final long _totalInvalidDocs;
private final long _totalDocs;
private final String _segmentCrc;
+ // Server's data CRC (forward index + dictionary checksum); null when the
server doesn't report it.
+ @Nullable
+ private final String _segmentDataCrc;
private final ValidDocIdsType _validDocIdsType;
private final long _segmentSizeInBytes;
private final long _segmentCreationTimeMillis;
private final String _instanceId;
private final ServiceStatus.Status _serverStatus;
+ public ValidDocIdsMetadataInfo(String segmentName, long totalValidDocs, long
totalInvalidDocs, long totalDocs,
+ String segmentCrc, ValidDocIdsType validDocIdsType, long
segmentSizeInBytes, long segmentCreationTimeMillis,
+ String instanceId, ServiceStatus.Status serverStatus) {
+ this(segmentName, totalValidDocs, totalInvalidDocs, totalDocs, segmentCrc,
validDocIdsType, segmentSizeInBytes,
+ segmentCreationTimeMillis, instanceId, serverStatus, null);
+ }
+
+ @JsonCreator
public ValidDocIdsMetadataInfo(@JsonProperty("segmentName") String
segmentName,
@JsonProperty("totalValidDocs") long totalValidDocs,
@JsonProperty("totalInvalidDocs") long totalInvalidDocs,
@JsonProperty("totalDocs") long totalDocs, @JsonProperty("segmentCrc")
String segmentCrc,
@JsonProperty("validDocIdsType") ValidDocIdsType validDocIdsType,
@JsonProperty("segmentSizeInBytes") long segmentSizeInBytes,
@JsonProperty("segmentCreationTimeMillis") long
segmentCreationTimeMillis,
- @JsonProperty("instanceId") String instanceId,
@JsonProperty("serverStatus") ServiceStatus.Status serverStatus) {
+ @JsonProperty("instanceId") String instanceId,
@JsonProperty("serverStatus") ServiceStatus.Status serverStatus,
+ @JsonProperty("segmentDataCrc") @Nullable String segmentDataCrc) {
Review Comment:
(minor) Keep the argument order consistent with declaration.
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java:
##########
@@ -747,6 +751,12 @@ private List<Map<String, Object>>
processValidDocIdsMetadata(String tableNameWit
validDocIdsMetadata.put("totalValidDocs", totalValidDocs);
validDocIdsMetadata.put("totalInvalidDocs", totalInvalidDocs);
validDocIdsMetadata.put("segmentCrc",
indexSegment.getSegmentMetadata().getCrc());
+ String reportableDataCrc = getReportableDataCrc(
Review Comment:
Same here. Directly use the one from segment metadata
##########
pinot-common/src/main/java/org/apache/pinot/common/restlet/resources/ValidDocIdsMetadataInfo.java:
##########
@@ -94,4 +110,11 @@ public String getInstanceId() {
public ServiceStatus.Status getServerStatus() {
return _serverStatus;
}
+
+ /// Server's data CRC, or null if not reported. Omitted from the payload
when null.
+ @JsonInclude(JsonInclude.Include.NON_NULL)
+ @Nullable
+ public String getSegmentDataCrc() {
Review Comment:
(minor) Keep the getter order consistent with declaration.
##########
pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java:
##########
@@ -574,7 +576,8 @@ public ValidDocIdsBitmapResponse downloadValidDocIdsBitmap(
byte[] validDocIdsBytes =
RoaringBitmapUtils.serialize(validDocIdSnapshot);
return new ValidDocIdsBitmapResponse(segmentName,
indexSegment.getSegmentMetadata().getCrc(),
finalValidDocIdsType, validDocIdsBytes,
_serverInstance.getInstanceDataManager().getInstanceId(),
- status);
+ status,
getReportableDataCrc(getSegmentZKMetadataOrNull(tableNameWithType, segmentName),
Review Comment:
We shouldn't need to read ZK metadata. Directly reading data crc from
SegmentMetadata should be good enough. For negative value, set it to `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]
