Re: [PR] Add offset based lag metrics [pinot]
Jackie-Jiang commented on PR #13298: URL: https://github.com/apache/pinot/pull/13298#issuecomment-2322120993 We are making one extra metadata read per message batch. Have you verified the overhead? Should we add a config to turn this on/off? -- 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: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add offset based lag metrics [pinot]
KKcorps merged PR #13298: URL: https://github.com/apache/pinot/pull/13298 -- 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: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add offset based lag metrics [pinot]
KKcorps commented on code in PR #13298:
URL: https://github.com/apache/pinot/pull/13298#discussion_r1631045755
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -174,6 +188,22 @@ private long getIngestionDelayMs(long ingestionTimeMs) {
return agedIngestionDelayMs;
}
+ private long getPartitionOffsetLag(IngestionOffsets offset) {
+if (offset == null) {
+ return 0;
+}
+StreamPartitionMsgOffset msgOffset = offset._offset;
+StreamPartitionMsgOffset latestOffset = offset._latestOffset;
+
+// Compute aged delay for current partition
+// TODO: Support other types of offsets
+if (!(msgOffset instanceof LongMsgOffset && latestOffset instanceof
LongMsgOffset)) {
+ return 0;
Review Comment:
Other than this, Kinesis returns a BigInteger offset and Pulsar returns a
string offset that is composed of 3 longs
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add offset based lag metrics [pinot]
KKcorps commented on code in PR #13298:
URL: https://github.com/apache/pinot/pull/13298#discussion_r1631036184
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -88,6 +90,16 @@ private static class IngestionTimestamps {
private final long _ingestionTimeMs;
private final long _firstStreamIngestionTimeMs;
}
+
+ private static class IngestionOffsets {
+IngestionOffsets(StreamPartitionMsgOffset offset, StreamPartitionMsgOffset
latestOffset) {
+ _offset = offset;
+ _latestOffset = latestOffset;
+}
+private final StreamPartitionMsgOffset _offset;
+private final StreamPartitionMsgOffset _latestOffset;
Review Comment:
yeah, i was just following existing style. changing
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add offset based lag metrics [pinot]
KKcorps commented on code in PR #13298:
URL: https://github.com/apache/pinot/pull/13298#discussion_r1631035110
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -174,6 +188,22 @@ private long getIngestionDelayMs(long ingestionTimeMs) {
return agedIngestionDelayMs;
}
+ private long getPartitionOffsetLag(IngestionOffsets offset) {
+if (offset == null) {
+ return 0;
+}
+StreamPartitionMsgOffset msgOffset = offset._offset;
Review Comment:
yes it is, let me rename this
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add offset based lag metrics [pinot]
KKcorps commented on code in PR #13298:
URL: https://github.com/apache/pinot/pull/13298#discussion_r1631034832
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -174,6 +188,22 @@ private long getIngestionDelayMs(long ingestionTimeMs) {
return agedIngestionDelayMs;
}
+ private long getPartitionOffsetLag(IngestionOffsets offset) {
+if (offset == null) {
Review Comment:
yeah we pass that when we want to reset the metric
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add offset based lag metrics [pinot]
KKcorps commented on code in PR #13298:
URL: https://github.com/apache/pinot/pull/13298#discussion_r1631032269
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -174,6 +188,22 @@ private long getIngestionDelayMs(long ingestionTimeMs) {
return agedIngestionDelayMs;
}
+ private long getPartitionOffsetLag(IngestionOffsets offset) {
Review Comment:
Yeah that's a good suggestion. I also don't feel that casting the LongOffset
is a good idea tbh and instead should be handled by consumer classes.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add offset based lag metrics [pinot]
KKcorps commented on code in PR #13298:
URL: https://github.com/apache/pinot/pull/13298#discussion_r1631032269
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -174,6 +188,22 @@ private long getIngestionDelayMs(long ingestionTimeMs) {
return agedIngestionDelayMs;
}
+ private long getPartitionOffsetLag(IngestionOffsets offset) {
Review Comment:
Yeah that's a good suggestion. I also don't feel that casting the LongOffset
is a good idea tbh.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add offset based lag metrics [pinot]
gortiz commented on code in PR #13298:
URL: https://github.com/apache/pinot/pull/13298#discussion_r1630692381
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -88,6 +90,16 @@ private static class IngestionTimestamps {
private final long _ingestionTimeMs;
private final long _firstStreamIngestionTimeMs;
}
+
+ private static class IngestionOffsets {
+IngestionOffsets(StreamPartitionMsgOffset offset, StreamPartitionMsgOffset
latestOffset) {
+ _offset = offset;
+ _latestOffset = latestOffset;
+}
+private final StreamPartitionMsgOffset _offset;
+private final StreamPartitionMsgOffset _latestOffset;
Review Comment:
nit: can we write the attributes before the constructor? Doing it the other
way around is very strange.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add offset based lag metrics [pinot]
swaminathanmanish commented on code in PR #13298:
URL: https://github.com/apache/pinot/pull/13298#discussion_r1630651998
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -174,6 +188,22 @@ private long getIngestionDelayMs(long ingestionTimeMs) {
return agedIngestionDelayMs;
}
+ private long getPartitionOffsetLag(IngestionOffsets offset) {
+if (offset == null) {
Review Comment:
Curious, can this even be null?
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -174,6 +188,22 @@ private long getIngestionDelayMs(long ingestionTimeMs) {
return agedIngestionDelayMs;
}
+ private long getPartitionOffsetLag(IngestionOffsets offset) {
+if (offset == null) {
+ return 0;
+}
+StreamPartitionMsgOffset msgOffset = offset._offset;
Review Comment:
This is current offset right?
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -174,6 +188,22 @@ private long getIngestionDelayMs(long ingestionTimeMs) {
return agedIngestionDelayMs;
}
+ private long getPartitionOffsetLag(IngestionOffsets offset) {
Review Comment:
This comment applies to time based reporting as well and can be taken as
follow up.
Should this belong to individual consumer types instead of common code just
like how getCurrentPartitionLagState is interfaced out, because reporting the
metric for all types here might not make any sense ?
Like you said, only for Kafka, this is relevant.
##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/IngestionDelayTracker.java:
##
@@ -174,6 +188,22 @@ private long getIngestionDelayMs(long ingestionTimeMs) {
return agedIngestionDelayMs;
}
+ private long getPartitionOffsetLag(IngestionOffsets offset) {
+if (offset == null) {
+ return 0;
+}
+StreamPartitionMsgOffset msgOffset = offset._offset;
+StreamPartitionMsgOffset latestOffset = offset._latestOffset;
+
+// Compute aged delay for current partition
+// TODO: Support other types of offsets
+if (!(msgOffset instanceof LongMsgOffset && latestOffset instanceof
LongMsgOffset)) {
+ return 0;
Review Comment:
What are other types that can fall here? Basically we need to be able to
explain this metric.
--
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: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
Re: [PR] Add offset based lag metrics [pinot]
codecov-commenter commented on PR #13298: URL: https://github.com/apache/pinot/pull/13298#issuecomment-2145015757 ## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/13298?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report Attention: Patch coverage is `0%` with `40 lines` in your changes are missing coverage. Please review. > Project coverage is 0.00%. Comparing base [(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`3396af7`)](https://app.codecov.io/gh/apache/pinot/commit/3396af7053f52f7fe86e3cb0c13762421445e57e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). > Report is 538 commits behind head on master. | [Files](https://app.codecov.io/gh/apache/pinot/pull/13298?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [...e/data/manager/realtime/IngestionDelayTracker.java](https://app.codecov.io/gh/apache/pinot/pull/13298?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fdata%2Fmanager%2Frealtime%2FIngestionDelayTracker.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvSW5nZXN0aW9uRGVsYXlUcmFja2VyLmphdmE=) | 0.00% | [27 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [...a/manager/realtime/RealtimeSegmentDataManager.java](https://app.codecov.io/gh/apache/pinot/pull/13298?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fdata%2Fmanager%2Frealtime%2FRealtimeSegmentDataManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVTZWdtZW50RGF0YU1hbmFnZXIuamF2YQ==) | 0.00% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [...a/org/apache/pinot/common/metrics/ServerGauge.java](https://app.codecov.io/gh/apache/pinot/pull/13298?src=pr&el=tree&filepath=pinot-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcommon%2Fmetrics%2FServerGauge.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vbWV0cmljcy9TZXJ2ZXJHYXVnZS5qYXZh) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [...ata/manager/realtime/RealtimeTableDataManager.java](https://app.codecov.io/gh/apache/pinot/pull/13298?src=pr&el=tree&filepath=pinot-core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpinot%2Fcore%2Fdata%2Fmanager%2Frealtime%2FRealtimeTableDataManager.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvUmVhbHRpbWVUYWJsZURhdGFNYW5hZ2VyLmphdmE=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/13298?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #13298 +/- ## = - Coverage 61.75%0.00% -61.76% = Files 2436 2459 +23 Lines133233 135600 +2367 Branches 2063620981 +345 = - Hits 822740-82274 - Misses44911 135600+90689 + Partials 60480 -6048 ``` | [Flag](https://app.codecov.io/gh/apache/pinot/pull/13298/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/13298/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=co
