PIP-187: Add API to analyze a subscription backlog and provide a accurate value #16545

eolivelli · 2022-07-12T11:07:46Z

PIP link: #16597
Also fixes #7623

Documentation

The new command is automatically documented for the REST API and for pulsar-admin

doc-not-needed

codelipenghui · 2022-07-12T13:04:52Z

The related PR support the precise backlog #14958

nicoloboschi

Nice work

I wonder if analise is a correct word in english, analyze should be the right verb

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java

...broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java

eolivelli

@nicoloboschi thanks for your review.
I have addressed your comments and also added one test about partitioned topics

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

dlg99

Overall LGTM, a couple of comments to address

conf/broker.conf

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java

dlg99

Looks good!

dlg99 · 2022-07-14T16:03:01Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java

+                } finally {
+                    entry.release();


nit: this could be finally block of outer try (on line 74)

.../java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

conf/broker.conf

codelipenghui · 2022-07-28T07:20:39Z

conf/broker.conf

+subscriptionBacklogScanMaxTimeMs=120000
+
+# Maximum number of entries to be read within a Analise backlog operation
+subscriptionBacklogScanMaxEntries=10000


I think we can support these 2 options in the http endpoint, instead of support ns/topic polices.

codelipenghui · 2022-07-28T12:25:21Z

managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java

+    @Test(timeOut = 20000)
+    void testScanSingleEntry() throws Exception {
+        testScan(10,1);
+    }
+
+    @Test(timeOut = 30000)
+    void testScanBatchesWithSomeRemainder() throws Exception {
+        testScan(10,3);
+    }
+
+    @Test(timeOut = 30000)
+    void testScanBatches() throws Exception {
+        testScan(10,5);
+    }
+
+    @Test(timeOut = 30000)
+    void testScanBatchWholeLedger() throws Exception {
+        testScan(10,1000);
+    }
+
+    @Test(timeOut = 1000)
+    void testScanBatchEmptyLedger() throws Exception {
+        testScan(0,10);
+    }


It's better to add a data provider instead of separate methods.

sure.
I usually prefer this form while developing tests, because it is easier to run single combination of parameters and the method name explains the meaning of the test.

I have updated the patch

codelipenghui · 2022-07-28T12:26:47Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */


Duplicated license header.

oh, I don't know how it happened. removed

codelipenghui · 2022-07-28T12:30:53Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

@@ -1569,6 +1590,63 @@ private void internalDeleteSubscriptionForNonPartitionedTopic(AsyncResponse asyn
            });
    }

+    private void internalAnalyzeSubscriptionBacklogForNonPartitionedTopic(AsyncResponse asyncResponse,


It's better to return CompletableFuture here to avoid passing the AsyncResponse to the internal of implement, it will help us to understand the returned type without looking at the implementation details

I agree with you, but this whole class is coded this way.
I am not sure it is worth to change the code style only for one method.

We have changed most of the admin implements with future returned types.

codelipenghui · 2022-07-28T12:32:45Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

+            if (!topicName.isPartitioned() && getPartitionedTopicMetadata(topicName,
+                    authoritative, false).partitions > 0) {
+                throw new RestException(Status.METHOD_NOT_ALLOWED,
+                        "Analyze backlog on a partitioned topic is not allowed, "
+                                + "please try do it on specific topic partition");
+            }


We should check the topic is a partitioned topic or not first? If it's a partitioned topic, the ownership checking is redundant

other methods check if the topic is partitioned in this same place.
if the broker is not the owner we will be redirected to another broker and we won't reach this point.

I notice that getPartitionedTopicMetadata is sync, I have reworked this part to make it fully async

codelipenghui · 2022-07-28T12:44:33Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java

+class OpScan implements ReadEntriesCallback {
+    private final ManagedCursorImpl cursor;
+    private final ManagedLedgerImpl ledger;
+    private final PositionImpl startPosition;


looks we can remove this one.

good catch, removed

eolivelli

@codelipenghui I have addressed your comments and posted some answers.

thanks

eolivelli · 2022-07-29T07:48:01Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java

+class OpScan implements ReadEntriesCallback {
+    private final ManagedCursorImpl cursor;
+    private final ManagedLedgerImpl ledger;
+    private final PositionImpl startPosition;


good catch, removed

eolivelli · 2022-07-29T07:51:48Z

managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java

+    @Test(timeOut = 20000)
+    void testScanSingleEntry() throws Exception {
+        testScan(10,1);
+    }
+
+    @Test(timeOut = 30000)
+    void testScanBatchesWithSomeRemainder() throws Exception {
+        testScan(10,3);
+    }
+
+    @Test(timeOut = 30000)
+    void testScanBatches() throws Exception {
+        testScan(10,5);
+    }
+
+    @Test(timeOut = 30000)
+    void testScanBatchWholeLedger() throws Exception {
+        testScan(10,1000);
+    }
+
+    @Test(timeOut = 1000)
+    void testScanBatchEmptyLedger() throws Exception {
+        testScan(0,10);
+    }


sure.
I usually prefer this form while developing tests, because it is easier to run single combination of parameters and the method name explains the meaning of the test.

I have updated the patch

eolivelli · 2022-07-29T07:52:54Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */


oh, I don't know how it happened. removed

eolivelli · 2022-07-29T07:56:44Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

@@ -1569,6 +1590,63 @@ private void internalDeleteSubscriptionForNonPartitionedTopic(AsyncResponse asyn
            });
    }

+    private void internalAnalyzeSubscriptionBacklogForNonPartitionedTopic(AsyncResponse asyncResponse,


I agree with you, but this whole class is coded this way.
I am not sure it is worth to change the code style only for one method.

eolivelli · 2022-07-29T08:09:55Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java

+            if (!topicName.isPartitioned() && getPartitionedTopicMetadata(topicName,
+                    authoritative, false).partitions > 0) {
+                throw new RestException(Status.METHOD_NOT_ALLOWED,
+                        "Analyze backlog on a partitioned topic is not allowed, "
+                                + "please try do it on specific topic partition");
+            }


other methods check if the topic is partitioned in this same place.
if the broker is not the owner we will be redirected to another broker and we won't reach this point.

I notice that getPartitionedTopicMetadata is sync, I have reworked this part to make it fully async

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java

eolivelli · 2022-07-29T10:16:18Z

@Technoboy- I have updated ManagedCursorImpl.
I won't change the API implementation at the moment.
We can follow up

…rate value (apache#16545)

eolivelli added area/broker area/tool doc-not-needed Your PR changes do not impact docs labels Jul 12, 2022

eolivelli added this to the 2.11.0 milestone Jul 12, 2022

eolivelli self-assigned this Jul 12, 2022

eolivelli mentioned this pull request Jul 12, 2022

Gets the exact number of message backlog for pulsar topic subscription #10705

Open

eolivelli requested review from codelipenghui, hangc0276 and lhotari July 12, 2022 19:42

codelipenghui added the wants/proposal label Jul 13, 2022

eolivelli force-pushed the impl/real-backlog branch from cbe6ac2 to b7bb192 Compare July 13, 2022 07:27

nicoloboschi requested changes Jul 13, 2022

View reviewed changes

eolivelli force-pushed the impl/real-backlog branch from 8318f6f to ff182d0 Compare July 13, 2022 12:17

eolivelli commented Jul 13, 2022

View reviewed changes

lhotari changed the title ~~Issue 7623: Add API to analise a subscription backlog and provide a accurate value~~ Issue 7623: Add API to analyse a subscription backlog and provide a accurate value Jul 13, 2022

dlg99 requested changes Jul 13, 2022

View reviewed changes

conf/broker.conf Outdated Show resolved Hide resolved

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java Outdated Show resolved Hide resolved

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java Outdated Show resolved Hide resolved

eolivelli changed the title ~~Issue 7623: Add API to analyse a subscription backlog and provide a accurate value~~ PIP-187: Add API to analyse a subscription backlog and provide a accurate value Jul 14, 2022

eolivelli force-pushed the impl/real-backlog branch from ff182d0 to d15e6b8 Compare July 14, 2022 07:39

eolivelli requested review from nicoloboschi and dlg99 July 14, 2022 07:40

dlg99 approved these changes Jul 14, 2022

View reviewed changes

eolivelli changed the title ~~PIP-187: Add API to analyse a subscription backlog and provide a accurate value~~ PIP-187: Add API to analyze a subscription backlog and provide a accurate value Jul 15, 2022

eolivelli force-pushed the impl/real-backlog branch from d15e6b8 to 9cb51d1 Compare July 15, 2022 12:11

mattisonchao requested review from mattisonchao and Technoboy- July 18, 2022 02:16

mattisonchao reviewed Jul 18, 2022

View reviewed changes

.../java/org/apache/pulsar/broker/service/persistent/PersistentDispatcherMultipleConsumers.java Outdated Show resolved Hide resolved

eolivelli force-pushed the impl/real-backlog branch from ba722e6 to 4c41c1b Compare July 18, 2022 07:52

Technoboy- reviewed Jul 18, 2022

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/OpScan.java Outdated Show resolved Hide resolved

Technoboy- reviewed Jul 18, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

eolivelli added 7 commits July 26, 2022 11:57

checkstyle

2b94806

checkstyle

82deeab

license

4bc6263

fix tests

0195754

checkstyle

bf897b3

add pulsar-admin command

24122ad

add ability to start from a position

bb35022

eolivelli force-pushed the impl/real-backlog branch from 5ba6e19 to bb35022 Compare July 26, 2022 09:57

eolivelli added 2 commits July 26, 2022 14:19

Allow to pass a position

33927e9

remove debug

e7e6ef5

eolivelli requested a review from michaeljmarshall July 26, 2022 12:26

mattisonchao reviewed Jul 26, 2022

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java Outdated Show resolved Hide resolved

eolivelli added 4 commits July 26, 2022 17:11

Address review comments

6ac0998

checkstyle

eb10310

checkstyle

1d282e4

checkstyle

5f91770

codelipenghui reviewed Jul 28, 2022

View reviewed changes

eolivelli added 2 commits July 29, 2022 09:44

Merge remote-tracking branch 'origin/master' into impl/real-backlog

8cc34b8

apply review comments

67873e0

eolivelli force-pushed the impl/real-backlog branch from fa14b9b to 67873e0 Compare July 29, 2022 08:21

eolivelli commented Jul 29, 2022

View reviewed changes

codelipenghui approved these changes Jul 29, 2022

View reviewed changes

Technoboy- reviewed Jul 29, 2022

View reviewed changes

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java Show resolved Hide resolved

ManagedCursorImpl scan - use position.orElseGet

6abcb9a

Technoboy- approved these changes Jul 29, 2022

View reviewed changes

checkstyle

0284062

Technoboy- merged commit 1af9695 into apache:master Jul 30, 2022

Gleiphir2769 pushed a commit to Gleiphir2769/pulsar that referenced this pull request Aug 4, 2022

PIP-187: Add API to analyze a subscription backlog and provide a accu…

870fa41

…rate value (apache#16545)

eolivelli deleted the impl/real-backlog branch September 1, 2022 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP-187: Add API to analyze a subscription backlog and provide a accurate value #16545

PIP-187: Add API to analyze a subscription backlog and provide a accurate value #16545

eolivelli commented Jul 12, 2022 •

edited

Loading

codelipenghui commented Jul 12, 2022

nicoloboschi left a comment

eolivelli left a comment

dlg99 left a comment

dlg99 left a comment

dlg99 Jul 14, 2022

codelipenghui Jul 28, 2022

codelipenghui Jul 28, 2022

eolivelli Jul 29, 2022

codelipenghui Jul 28, 2022

eolivelli Jul 29, 2022

codelipenghui Jul 28, 2022

eolivelli Jul 29, 2022

Technoboy- Jul 29, 2022

codelipenghui Jul 28, 2022

eolivelli Jul 29, 2022

codelipenghui Jul 28, 2022

eolivelli Jul 29, 2022

eolivelli left a comment

eolivelli Jul 29, 2022

eolivelli Jul 29, 2022

eolivelli Jul 29, 2022

eolivelli Jul 29, 2022

eolivelli Jul 29, 2022

eolivelli commented Jul 29, 2022

PIP-187: Add API to analyze a subscription backlog and provide a accurate value #16545

PIP-187: Add API to analyze a subscription backlog and provide a accurate value #16545

Conversation

eolivelli commented Jul 12, 2022 • edited Loading

Documentation

codelipenghui commented Jul 12, 2022

nicoloboschi left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

dlg99 left a comment

Choose a reason for hiding this comment

dlg99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli commented Jul 29, 2022

eolivelli commented Jul 12, 2022 •

edited

Loading