[GitHub] [pulsar] cckellogg commented on a change in pull request #10853: [Issue #9488][add admin api getBacklogSize]

[GitBox <gi...@apache.org>]

cckellogg commented on a change in pull request #10853:
URL: https://github.com/apache/pulsar/pull/10853#discussion_r646956591



##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/PersistentTopics.java
##########
@@ -662,6 +662,35 @@ public PersistentOfflineTopicStats getBacklog(@PathParam("property") String prop
         return internalGetBacklog(authoritative);
     }
 
+    @GET
+    @Path("/{property}/{cluster}/{namespace}/{topic}/ledger/{ledgerId}/entry/{entryId}/checkLedgerExists/{checkLedgerExists}/backlogSize")
+    @ApiOperation(value = "Calculate backlog size given a messageId.")
+    @ApiResponses(value = {
+            @ApiResponse(code = 307, message = "Current broker doesn't serve the namespace of this topic"),
+            @ApiResponse(code = 401, message = "Don't have permission to administrate resources on this tenant"),

Review comment:
       This should be not authenticated 

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v1/PersistentTopics.java
##########
@@ -662,6 +662,35 @@ public PersistentOfflineTopicStats getBacklog(@PathParam("property") String prop
         return internalGetBacklog(authoritative);
     }
 
+    @GET
+    @Path("/{property}/{cluster}/{namespace}/{topic}/ledger/{ledgerId}/entry/{entryId}/checkLedgerExists/{checkLedgerExists}/backlogSize")

Review comment:
       To me this is a very complicated api. Where is a user suppose to get ledger Id and entry Id? Can just a message Id work? what is the purpose of this `checkLedgerExists`?

##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java
##########
@@ -26,6 +26,9 @@
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.google.common.collect.Sets;
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+import com.google.gson.JsonObject;

Review comment:
       There is already a jsonMapper `jsonMapper()` no need to add this dependency.

##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1596,6 +1596,36 @@ void createSubscription(String topic, String subscriptionName, MessageId message
     Map<BacklogQuota.BacklogQuotaType, BacklogQuota> getBacklogQuotaMap(String topic, boolean applied)
             throws PulsarAdminException;
 
+    /**
+     * Get backlog size by its messageId.
+     * @param topic
+     *            Topic name
+     * @param ledgerId
+     *            Ledger id
+     * @param entryId
+     *            Entry id
+     * @param checkLedgerExists
+     *            checkLedgerExists
+     * @return the backlog size from
+     * @throws PulsarAdminException
+     *            Unexpected error
+     */
+    Long getBacklogSizeByMessageId(String topic, long ledgerId, long entryId, boolean checkLedgerExists) throws PulsarAdminException;

Review comment:
       Why not pass a messageID? Same for below.

##########
File path: pulsar-client-admin-api/src/main/java/org/apache/pulsar/client/admin/Topics.java
##########
@@ -1596,6 +1596,36 @@ void createSubscription(String topic, String subscriptionName, MessageId message
     Map<BacklogQuota.BacklogQuotaType, BacklogQuota> getBacklogQuotaMap(String topic, boolean applied)
             throws PulsarAdminException;
 
+    /**
+     * Get backlog size by its messageId.

Review comment:
       What is backlog here? The number of messages or the size in bytes?




-- 
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:
users@infra.apache.org