Skip to content
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

[fix][broker] Fix creating producer failure when set backlog quota. #15663

Merged
merged 4 commits into from
May 25, 2022

Conversation

Technoboy-
Copy link
Contributor

Motivation

When trying to reproduce the problem of #15609 using the master's code, it was found that the master also had this bug. The root cause is:
When there is only one ledger in the ManagedLedger, after the current ledger is closed, it has the timestamp and exceeds the time set by the backlog-qutoa, resulting in the failure to create the producer.

The added test could reproduce this.

So when there is only one ledger, we should not exclude it.

Verifying this change

If revert this patch, the added test will fail.

Documentation

  • no-need-doc
    (Please explain why)

@Technoboy- Technoboy- force-pushed the producer-exceed-quota-1 branch 2 times, most recently from 5c6c4d9 to 73ccd91 Compare May 23, 2022 09:09
@Technoboy- Technoboy- closed this May 23, 2022
@Technoboy- Technoboy- reopened this May 23, 2022
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left a minor comment.

Comment on lines +2576 to +2573
org.apache.bookkeeper.mledger.proto.MLDataFormats.ManagedLedgerInfo.LedgerInfo
ledgerInfo = ledger.getLedgerInfo(ledgerId).get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here will be a risk to call .get() directly since the implementation of getLedgerInfo() might changed although this does not appear to be a problem at present.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think we can use another PR to make it asynchronous.

@Technoboy- Technoboy- force-pushed the producer-exceed-quota-1 branch from 73ccd91 to 9ce00b3 Compare May 25, 2022 02:07
@Technoboy- Technoboy- force-pushed the producer-exceed-quota-1 branch from 9ce00b3 to d93ae3c Compare May 25, 2022 02:10
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If revert this patch, the added test will fail.

But it still passed in my env without this patch even with the previous test.

image

@Technoboy-
Copy link
Contributor Author

If revert this patch, the added test will fail.

But it still passed in my env without this patch even with the previous test.

image

Nothing changed except the rollover time.

@Technoboy-
Copy link
Contributor Author

If revert this patch, the added test will fail.

But it still passed in my env without this patch even with the previous test.
image

Nothing changed except the rollover time.

Rollover time can set any value, the test need to sleep more than it.

@BewareMyPower
Copy link
Contributor

I applied two changes, set a breakpoint, then debugged the test, but it never went to the breakpoint.

image

@Technoboy-
Copy link
Contributor Author

I applied two changes, set a breakpoint, then debugged the test, but it never went to the breakpoint.

image

Yes, I have updated the test, if we set the ledger config, some internal tasks don't update, and the main task of rollover the ledger using the old scheduled time. Now it's ok all. Thanks for testing so carefully and for your kind suggestion.

Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@BewareMyPower BewareMyPower merged commit 3a80458 into apache:master May 25, 2022
codelipenghui pushed a commit that referenced this pull request May 26, 2022
…15663)

When trying to reproduce the problem of #15609 using the master's code, it was found that the master also had this bug. The root cause is:
When there is only one ledger in the ManagedLedger, after the current ledger is closed, it has the timestamp and exceeds the time set by the backlog-qutoa, resulting in the failure to create the producer.

The added test could reproduce this.

So when there is only one ledger, we should not exclude it.

If revert this patch, the added test will fail.

(cherry picked from commit 3a80458)
@codelipenghui codelipenghui added the cherry-picked/branch-2.8 Archived: 2.8 is end of life label May 26, 2022
codelipenghui pushed a commit that referenced this pull request May 26, 2022
…15663)

When trying to reproduce the problem of #15609 using the master's code, it was found that the master also had this bug. The root cause is:
When there is only one ledger in the ManagedLedger, after the current ledger is closed, it has the timestamp and exceeds the time set by the backlog-qutoa, resulting in the failure to create the producer.

The added test could reproduce this.

So when there is only one ledger, we should not exclude it.

If revert this patch, the added test will fail.

(cherry picked from commit 3a80458)
codelipenghui pushed a commit that referenced this pull request May 26, 2022
…15663)

When trying to reproduce the problem of #15609 using the master's code, it was found that the master also had this bug. The root cause is:
When there is only one ledger in the ManagedLedger, after the current ledger is closed, it has the timestamp and exceeds the time set by the backlog-qutoa, resulting in the failure to create the producer.

The added test could reproduce this.

So when there is only one ledger, we should not exclude it.

If revert this patch, the added test will fail.

(cherry picked from commit 3a80458)
nicoloboschi added a commit to nicoloboschi/pulsar that referenced this pull request May 26, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 26, 2022
…pache#15663)

When trying to reproduce the problem of apache#15609 using the master's code, it was found that the master also had this bug. The root cause is:
When there is only one ledger in the ManagedLedger, after the current ledger is closed, it has the timestamp and exceeds the time set by the backlog-qutoa, resulting in the failure to create the producer.

The added test could reproduce this.

So when there is only one ledger, we should not exclude it.

If revert this patch, the added test will fail.

(cherry picked from commit 3a80458)
(cherry picked from commit cc8895e)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jun 7, 2022
…pache#15663)

When trying to reproduce the problem of apache#15609 using the master's code, it was found that the master also had this bug. The root cause is:
When there is only one ledger in the ManagedLedger, after the current ledger is closed, it has the timestamp and exceeds the time set by the backlog-qutoa, resulting in the failure to create the producer.

The added test could reproduce this.

So when there is only one ledger, we should not exclude it.

If revert this patch, the added test will fail.

(cherry picked from commit 3a80458)
(cherry picked from commit dc7d512)
@Technoboy- Technoboy- deleted the producer-exceed-quota-1 branch August 10, 2022 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.8.4 release/2.9.3 release/2.10.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants