MarvinCai commented on a change in pull request #9302:
URL: https://github.com/apache/pulsar/pull/9302#discussion_r563416467
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -1826,6 +1826,13 @@
)
private boolean exposePreciseBacklogInPrometheus = false;
+ @FieldContext(
+ category = CATEGORY_METRICS,
+ doc = "Enable expose the backlog size fro each subscription when
generating stats.\n" +
+ " Locking is used for fetching the status so default to
false."
+ )
+ private boolean exposeSubscriptionBacklokSizeInPrometheus = false;
Review comment:
will fix.
##########
File path:
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -764,10 +766,11 @@ default void delete(String topic, boolean force) throws
PulsarAdminException {
* @throws PulsarAdminException
* Unexpected error
*/
- TopicStats getStats(String topic, boolean getPreciseBacklog) throws
PulsarAdminException;
+ TopicStats getStats(String topic, boolean getPreciseBacklog,
Review comment:
@zymap
From what I've seen, this is only used by Admin CLI
([CmdTopics.java](https://github.com/apache/pulsar/blob/master/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdTopics.java#L483-L490)),
which provides default value <false>. for all needed parameters, e.g.
getPreciseBacklog and subscriptionBacklogSize. And there's another default
implementation of [TopicStats getStats(String
topic)](https://github.com/apache/pulsar/blob/master/pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/Topics.java#L769-L771)
which is used in some other places, it also has default value <false> provided
for getPreciseBacklog and subscriptionBacklogSize.
So it shouldn't create compatibility issue for users.
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -1826,6 +1826,13 @@
)
private boolean exposePreciseBacklogInPrometheus = false;
+ @FieldContext(
+ category = CATEGORY_METRICS,
+ doc = "Enable expose the backlog size fro each subscription when
generating stats.\n" +
+ " Locking is used for fetching the status so default to
false."
+ )
+ private boolean exposeSubscriptionBacklokSizeInPrometheus = false;
Review comment:
I think it is actually quiet expensive, it's using existing
[estimateBacklogFromPosition(PositionImpl
pos)](https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1120)
to get the backlog size, which requires locking the whole MLedger object. And
this will happen for every subscription, so periodic metric generation with
this subscription backlog size could affect performance, so I think we should
add a flag for it and disable by default.
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -1826,6 +1826,13 @@
)
private boolean exposePreciseBacklogInPrometheus = false;
+ @FieldContext(
+ category = CATEGORY_METRICS,
+ doc = "Enable expose the backlog size fro each subscription when
generating stats.\n" +
+ " Locking is used for fetching the status so default to
false."
+ )
+ private boolean exposeSubscriptionBacklokSizeInPrometheus = false;
Review comment:
I think it is actually quiet expensive, it's using existing
[estimateBacklogFromPosition(PositionImpl
pos)](https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1120)
to get the backlog size, which requires locking the whole MLedger object. And
this will happen for every subscription, periodic metric generation with this
subscription backlog size could affect performance, so I think we should add a
flag for it and disable by default.
##########
File path:
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
##########
@@ -1826,6 +1826,13 @@
)
private boolean exposePreciseBacklogInPrometheus = false;
+ @FieldContext(
+ category = CATEGORY_METRICS,
+ doc = "Enable expose the backlog size fro each subscription when
generating stats.\n" +
+ " Locking is used for fetching the status so default to
false."
+ )
+ private boolean exposeSubscriptionBacklokSizeInPrometheus = false;
Review comment:
@codelipenghui
I think it is actually quiet expensive, it's using existing
[estimateBacklogFromPosition(PositionImpl
pos)](https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java#L1120)
to get the backlog size, which requires locking the whole MLedger object. And
this will happen for every subscription, periodic metric generation with this
subscription backlog size could affect performance, so I think we should add a
flag for it and disable by default.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
