-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose lastConsumedTimestamp and lastAckedTimestamp to consumer stats #6051
Conversation
--- Master Issue: apache#6046 *Motivation* Make people can use the timestamp to tell if acknowledge and consumption are happening. *Modifications* - Add lastConsumedTimestamp and lastAckedTimestamp to consume stats *Verify this change* - Pass the test `testConsumerStatsLastTimestamp`
@sijie PTAL. And please help me to rerun the test. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good to me. although I think we should expose these two metrics to subscription stats as well.
434440a
to
567e6e5
Compare
@sijie I exposed the metrics to the subscription stats. Please take a look when you have time. |
retest this please |
1 similar comment
retest this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zymap overall looks good to me. Please take a look at my comments regarding the test. Also we need to add documentation for these metrics.
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiTest2.java
Show resolved
Hide resolved
bca224a
to
1711c4c
Compare
@sijie Thanks for your remind. I fix the tests and add the stats to the doc. Please take a look when you are free. |
retest this please |
run java8 tests |
run java8 tests |
…apache#6051) --- Master Issue: apache#6046 *Motivation* Make people can use the timestamp to tell if acknowledge and consumption are happening. *Modifications* - Add lastConsumedTimestamp and lastAckedTimestamp to consume stats *Verify this change* - Pass the test `testConsumerStatsLastTimestamp`
…apache#6051) --- Master Issue: apache#6046 *Motivation* Make people can use the timestamp to tell if acknowledge and consumption are happening. *Modifications* - Add lastConsumedTimestamp and lastAckedTimestamp to consume stats *Verify this change* - Pass the test `testConsumerStatsLastTimestamp` (cherry picked from commit 5728977)
…#6051) --- Master Issue: #6046 *Motivation* Make people can use the timestamp to tell if acknowledge and consumption are happening. *Modifications* - Add lastConsumedTimestamp and lastAckedTimestamp to consume stats *Verify this change* - Pass the test `testConsumerStatsLastTimestamp` (cherry picked from commit 5728977)
…apache#6051) --- Master Issue: apache#6046 *Motivation* Make people can use the timestamp to tell if acknowledge and consumption are happening. *Modifications* - Add lastConsumedTimestamp and lastAckedTimestamp to consume stats *Verify this change* - Pass the test `testConsumerStatsLastTimestamp` (cherry picked from commit 5728977)
…apache#6051) --- Master Issue: apache#6046 *Motivation* Make people can use the timestamp to tell if acknowledge and consumption are happening. *Modifications* - Add lastConsumedTimestamp and lastAckedTimestamp to consume stats *Verify this change* - Pass the test `testConsumerStatsLastTimestamp`
Master Issue: #6046
Motivation
Make people can use the timestamp to tell if acknowledge and consumption
are happening.
Modifications
Verify this change
testConsumerStatsLastTimestamp