[fix][broker]unify time unit at dropping the backlog on a topic #17957

HQebupt · 2022-10-07T03:42:52Z

Motivation

The backlog quota in time does not work due to time unit inconsistency in dropping the backlog on a topic.
The unit of backlog quota limit in time is seconds.

pulsar/pulsar-client-admin-api/src/main/java/org/apache/pulsar/common/policies/data/BacklogQuota.java

Lines 47 to 52 in 8d13ff8

    
               /** 
        
                * Gets quota limit in time. 
        
                * 
        
                * @return quota limit in second 
        
                */ 
        
               int getLimitTime();

The time unit inconsistency is here as follows.

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BacklogQuotaManager.java

Lines 221 to 224 in 8d13ff8

    
           // Timestamp only > 0 if ledger has been closed 
        
           if (ledgerInfo.getTimestamp() > 0 
        
                   && currentMillis - ledgerInfo.getTimestamp() > quota.getLimitTime()) { 
        
               // skip whole ledger for the slowest cursor

Modifications

unify time unit.

Verifying this change

Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly.

Need to update docs?

doc-required
doc-not-needed
doc
doc-complete

Matching PR in forked repository

PR in forked repository: HQebupt#3

HQebupt · 2022-10-07T03:45:42Z

@codelipenghui @Technoboy- @eolivelli @Jason918 @AnonHxy @gaozhangmin PTAL

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BacklogQuotaManagerTest.java

HQebupt · 2022-10-08T02:13:38Z

@AnonHxy PTAL

HQebupt · 2022-10-08T06:36:03Z

/pulsarbot ready-to-test

HQebupt · 2022-10-08T09:32:35Z

/pulsarbot run-failure-checks

HQebupt · 2022-10-09T02:09:08Z

/pulsarbot ready-to-test

HQebupt · 2022-10-09T09:21:35Z

/pulsarbot run-failure-checks

HQebupt · 2022-10-09T11:08:42Z

/pulsarbot run-failure-checks

HQebupt · 2022-10-09T12:58:58Z

/pulsarbot run-failure-checks

codelipenghui

Nice catch!

codelipenghui · 2022-10-10T03:02:25Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BacklogQuotaManager.java

@@ -220,7 +220,7 @@ private void dropBacklogForTimeLimit(PersistentTopic persistentTopic, BacklogQuo
                    }
                    // Timestamp only > 0 if ledger has been closed
                    if (ledgerInfo.getTimestamp() > 0
-                            && currentMillis - ledgerInfo.getTimestamp() > quota.getLimitTime()) {
+                            && currentMillis - ledgerInfo.getTimestamp() > quota.getLimitTime() * 1000) {


It's a confusing name :)

We should deprecate the old name limitTime and introduce a new one limitTimeInSec
Not for this PR, we can start a new discussion in the dev mailing list and make it only happen on the next major release @HQebupt

(cherry picked from commit add77aa)

### Motivation fix cherry-pick #17609 import fix cherry-pick #17957 import fix cherry-pick #16878 lose problem fix cherry-pick #17503 problem

(cherry picked from commit add77aa)

…pping the backlog on a topic)

…he#17957) (cherry picked from commit add77aa) (cherry picked from commit 49e86d9)

…at dropping the backlog on a topic) (cherry picked from commit 95748ca)

…he#17957) (cherry picked from commit add77aa) (cherry picked from commit 49e86d9)

…at dropping the backlog on a topic) (cherry picked from commit 95748ca)

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 7, 2022

AnonHxy assigned HQebupt Oct 7, 2022

AnonHxy reviewed Oct 7, 2022

View reviewed changes

HQebupt requested a review from AnonHxy October 8, 2022 02:13

AnonHxy approved these changes Oct 8, 2022

View reviewed changes

AnonHxy requested review from codelipenghui, Jason918 and Technoboy- October 8, 2022 06:09

AnonHxy added type/bug The PR fixed a bug or issue reported a bug cherry-picked/branch-2.10 cherry-picked/branch-2.11 labels Oct 8, 2022

Jason918 approved these changes Oct 8, 2022

View reviewed changes

HQebupt added 2 commits October 8, 2022 19:57

[fix][broker]unify time unit at dropping the backlog on a topic

9cdc29a

[fix][broker]unify time unit at dropping the backlog on a topic

0c9bb90

HQebupt force-pushed the timeFix branch from 7da7c9f to 0c9bb90 Compare October 8, 2022 11:58

HQebupt closed this Oct 8, 2022

HQebupt reopened this Oct 8, 2022

Jason918 added release/2.11.1 release/2.10.3 and removed cherry-picked/branch-2.10 cherry-picked/branch-2.11 labels Oct 10, 2022

codelipenghui added this to the 2.12.0 milestone Oct 10, 2022

codelipenghui approved these changes Oct 10, 2022

View reviewed changes

codelipenghui merged commit add77aa into apache:master Oct 10, 2022

codelipenghui added the release/2.9.4 label Oct 10, 2022

congbobo184 pushed a commit that referenced this pull request Nov 14, 2022

[fix][broker]unify time unit at dropping the backlog on a topic (#17957)

d6c8566

(cherry picked from commit add77aa)

congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 14, 2022

congbobo184 pushed a commit that referenced this pull request Dec 7, 2022

[fix][broker]unify time unit at dropping the backlog on a topic (#17957)

9d2b51b

(cherry picked from commit add77aa)

congbobo184 mentioned this pull request Dec 7, 2022

[cherry-pick][branch-2.9] fix cherry-pick #17609 #17957 import #18793

Merged

liangyepianzhou pushed a commit that referenced this pull request Dec 8, 2022

[fix][broker]unify time unit at dropping the backlog on a topic (#17957)

49e86d9

(cherry picked from commit add77aa)

liangyepianzhou added a commit that referenced this pull request Dec 8, 2022

[cherry-pick][branch-2.10] cherry-pick #17957 (unify time unit at dro…

95748ca

…pping the backlog on a topic)

liangyepianzhou added the cherry-picked/branch-2.10 label Dec 8, 2022

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023

[fix][broker]unify time unit at dropping the backlog on a topic (apac…

ef37bf5

…he#17957) (cherry picked from commit add77aa) (cherry picked from commit 49e86d9)

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023

[cherry-pick][branch-2.10] cherry-pick apache#17957 (unify time unit …

ca85a41

…at dropping the backlog on a topic) (cherry picked from commit 95748ca)

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023

[fix][broker]unify time unit at dropping the backlog on a topic (apac…

9518891

…he#17957) (cherry picked from commit add77aa) (cherry picked from commit 49e86d9)

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023

[cherry-pick][branch-2.10] cherry-pick apache#17957 (unify time unit …

d975937

…at dropping the backlog on a topic) (cherry picked from commit 95748ca)

Technoboy- pushed a commit that referenced this pull request Feb 8, 2023

[fix][broker]unify time unit at dropping the backlog on a topic (#17957)

627b96f

Technoboy- added the cherry-picked/branch-2.11 label Feb 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][broker]unify time unit at dropping the backlog on a topic #17957

[fix][broker]unify time unit at dropping the backlog on a topic #17957

HQebupt commented Oct 7, 2022

HQebupt commented Oct 7, 2022

HQebupt commented Oct 8, 2022

HQebupt commented Oct 8, 2022

HQebupt commented Oct 8, 2022

HQebupt commented Oct 9, 2022

HQebupt commented Oct 9, 2022

HQebupt commented Oct 9, 2022

HQebupt commented Oct 9, 2022

codelipenghui left a comment

codelipenghui Oct 10, 2022 •

edited

Loading

	/**
	* Gets quota limit in time.
	*
	* @return quota limit in second
	*/
	int getLimitTime();

	// Timestamp only > 0 if ledger has been closed
	if (ledgerInfo.getTimestamp() > 0
	&& currentMillis - ledgerInfo.getTimestamp() > quota.getLimitTime()) {
	// skip whole ledger for the slowest cursor

[fix][broker]unify time unit at dropping the backlog on a topic #17957

[fix][broker]unify time unit at dropping the backlog on a topic #17957

Conversation

HQebupt commented Oct 7, 2022

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

HQebupt commented Oct 7, 2022

HQebupt commented Oct 8, 2022

HQebupt commented Oct 8, 2022

HQebupt commented Oct 8, 2022

HQebupt commented Oct 9, 2022

HQebupt commented Oct 9, 2022

HQebupt commented Oct 9, 2022

HQebupt commented Oct 9, 2022

codelipenghui left a comment

Choose a reason for hiding this comment

codelipenghui Oct 10, 2022 • edited Loading

Choose a reason for hiding this comment

codelipenghui Oct 10, 2022 •

edited

Loading