noob-se7en commented on code in PR #17749:
URL: https://github.com/apache/pinot/pull/17749#discussion_r2882990359
##########
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerGauge.java:
##########
@@ -99,6 +99,8 @@ public enum ServerGauge implements AbstractMetrics.Gauge {
REALTIME_INGESTION_DELAY_MS("milliseconds", false,
"The difference of the current timestamp and the timestamp present in
the last consumed message record."),
END_TO_END_REALTIME_INGESTION_DELAY_MS("milliseconds", false),
+ REALTIME_INGESTION_DELAY_HAS_DATA("boolean", false,
Review Comment:
nit:
How about `REALTIME_INGESTION_DELAY_REPORTING_STATUS` instead of
`REALTIME_INGESTION_DELAY_HAS_DATA` ?
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##########
@@ -551,16 +555,16 @@ public long getPartitionIngestionTimeMs(int partitionId) {
*
* @param partitionId partition for which we are retrieving the delay
*
- * @return End to end ingestion delay in milliseconds for the given
partition ID.
+ * @return End to end ingestion delay in milliseconds for the given
partition ID,
+ * or null if first stream ingestion time is not available for the partition.
*/
- public long getPartitionEndToEndIngestionDelayMs(int partitionId) {
+ public Long getPartitionEndToEndIngestionDelayMs(int partitionId) {
Review Comment:
nit:
```suggestion
@Nullable
public Long getPartitionEndToEndIngestionDelayMs(int partitionId) {
```
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##########
@@ -570,20 +574,38 @@ public long getPartitionEndToEndIngestionDelayMs(int
partitionId) {
*
* @param partitionId partition for which we are retrieving the delay
*
- * @return ingestion delay in milliseconds for the given partition ID.
+ * @return ingestion delay in milliseconds for the given partition ID,
+ * or null if ingestion time is not available for the partition.
*/
- public long getPartitionIngestionDelayMs(int partitionId) {
+ public Long getPartitionIngestionDelayMs(int partitionId) {
Review Comment:
Nit:
```suggestion
@Nullable
public Long getPartitionIngestionDelayMs(int partitionId) {
```
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##########
@@ -570,20 +574,38 @@ public long getPartitionEndToEndIngestionDelayMs(int
partitionId) {
*
* @param partitionId partition for which we are retrieving the delay
*
- * @return ingestion delay in milliseconds for the given partition ID.
+ * @return ingestion delay in milliseconds for the given partition ID,
+ * or null if ingestion time is not available for the partition.
*/
- public long getPartitionIngestionDelayMs(int partitionId) {
+ public Long getPartitionIngestionDelayMs(int partitionId) {
IngestionInfo ingestionInfo = _ingestionInfoMap.get(partitionId);
- long ingestionTimeMs = 0;
- if ((ingestionInfo != null) && (ingestionInfo._ingestionTimeMs > 0)) {
- ingestionTimeMs = ingestionInfo._ingestionTimeMs;
+ if (ingestionInfo == null || ingestionInfo._ingestionTimeMs < 0) {
+ return null;
}
// Compute aged delay for current partition
- long agedIngestionDelayMs = _clock.millis() - ingestionTimeMs;
+ long agedIngestionDelayMs = _clock.millis() -
ingestionInfo._ingestionTimeMs;
// Correct to zero for any time shifts due to NTP or time reset.
return Math.max(agedIngestionDelayMs, 0);
}
+ /**
+ * Method to get if ingestion delay data is available for the given
partition (i.e. ingestion info has been
+ * reported and both timestamps are valid)
+ *
+ * @param partitionId partition for which we are checking data availability
+ *
+ * @return 1 if ingestion delay data is available, 0 otherwise
+ */
+ public long getPartitionIngestionDelayHasData(int partitionId) {
+ IngestionInfo ingestionInfo = _ingestionInfoMap.get(partitionId);
+ if (ingestionInfo == null
+ || ingestionInfo._ingestionTimeMs < 0
+ || ingestionInfo._firstStreamIngestionTimeMs < 0) {
+ return 0;
Review Comment:
We should only return 0 here when `ingestionInfo == null`.
Since `ingestionInfo == null ` can happen when consumer fails to report its
status and thats only when `REALTIME_INGESTION_DELAY_HAS_DATA` should be set.
If consumer reports the ingestion status successfully still
`ingestionInfo._firstStreamIngestionTimeMs < 0` or
`ingestionInfo._ingestionTimeMs < 0` can happen and for that we dont want to
get an alert.
##########
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##########
@@ -570,20 +574,38 @@ public long getPartitionEndToEndIngestionDelayMs(int
partitionId) {
*
* @param partitionId partition for which we are retrieving the delay
*
- * @return ingestion delay in milliseconds for the given partition ID.
+ * @return ingestion delay in milliseconds for the given partition ID,
+ * or null if ingestion time is not available for the partition.
*/
- public long getPartitionIngestionDelayMs(int partitionId) {
+ public Long getPartitionIngestionDelayMs(int partitionId) {
IngestionInfo ingestionInfo = _ingestionInfoMap.get(partitionId);
- long ingestionTimeMs = 0;
- if ((ingestionInfo != null) && (ingestionInfo._ingestionTimeMs > 0)) {
- ingestionTimeMs = ingestionInfo._ingestionTimeMs;
+ if (ingestionInfo == null || ingestionInfo._ingestionTimeMs < 0) {
+ return null;
Review Comment:
I dont think this should cause NPE during metric reporting but will be good
to confirm.
My understanding is Yammer metric reporter should return null metric as it
is and prometheus JMX exporter will just skip the metric so time series graph
for this metric will have a gap. This should be okay if this is the case.
--
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]
