Skip to content

Revert "Change rollCurrentLedgerIfFull logic to follow lazy creation of ledger (#14672) #16806

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.

Sign up for GitHub

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

Jump to bottom
Merged
Technoboy- merged 5 commits into from
Aug 4, 2022
Merged

Revert "Change rollCurrentLedgerIfFull logic to follow lazy creation of ledger (#14672) #16806

Technoboy- merged 5 commits into from
Aug 4, 2022

Conversation

@mattisonchao
Copy link
Member

@mattisonchao mattisonchao commented Jul 27, 2022 •
edited
Loading

Motivation

The last created topic ledger will be never removed because the trimming strategy will not remove the current ledger. In such a scenario, we will maintain a lot of useless data if the topic is no longer being used. Moreover, it may cause disk problems if we keep a lot mount of discarded topics in the cluster.

Modifications

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc-not-needed
    (Please explain why)

BewareMyPower
BewareMyPower reviewed Jul 27, 2022
View reviewed changes
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.

we will maintain a lot of useless data if the topic is no longer being used

Could you add a test for it?

@mattisonchao mattisonchao added the release/blocker Indicate the PR or issue that should block the release until it gets resolved label Jul 27, 2022
@mattisonchao mattisonchao added this to the 2.11.0 milestone Jul 27, 2022
@BewareMyPower
Copy link
Contributor

BewareMyPower commented Jul 27, 2022

I found testGetNumberOfEntriesInStorage might rely on #14672. Did you have any thought about #15627 (comment)?

BewareMyPower
BewareMyPower requested changes Jul 28, 2022
View reviewed changes
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.

testGetNumberOfEntriesInStorage also failed. I think it's caused by the behavior change. BTW, we should verify the managed ledger's state, which should be LedgerOpened instead of LedgerClosed.

@BewareMyPower BewareMyPower mentioned this pull request Jul 28, 2022
Closed
@Nicklee007
Copy link
Contributor

Nicklee007 commented Jul 29, 2022 •
edited
Loading

testGetNumberOfEntriesInStorage also failed. I think it's caused by the behavior change. BTW, we should verify the managed ledger's state, which should be LedgerOpened instead of LedgerClosed.

@mattisonchao @BewareMyPower How about through the PR #15474 fix those unit test and achieve the same effect like revert.

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Jul 29, 2022

@Nicklee007 I will take a look

@BewareMyPower BewareMyPower mentioned this pull request Jul 29, 2022
Open
1 task
@mattisonchao
Copy link
Member Author

mattisonchao commented Jul 30, 2022

I found testGetNumberOfEntriesInStorage might rely on #14672. Did you have any thought about #15627 (comment)?

We can fix this test to satisfy this revert.

mattisonchao and others added 5 commits August 3, 2022 16:30
@mattisonchao
Revert "Change rollCurrentLedgerIfFull logic to follow lazy creat…
61ff3a8 
…ion of ledger (#14672)"

This reverts commit 30f7f00.
@poorbarcode @mattisonchao
[improve] [txn] [PIP-160] Transaction log store enable the batch feat…
22d1229 
…ure (#16685)

Master Issue: #15370

### Motivation

see #15370

### Modifications

I will complete proposal #15370 with these pull requests( *current pull request is the step-3* ):

1. Write the batch transaction log handler: `TxnLogBufferedWriter`
2. Configuration changes and protocol changes.
3. Transaction log store enables the batch feature.
4. Pending ack log store enables the batch feature.
5. Supports dynamic configuration.
6. Append admin API for transaction batch log and docs( admin and configuration doc ).
7. Append metrics support for transaction batch log.
@mattisonchao
Fix test
da96b20
@mattisonchao
Fix rebase problem
0d7aef4
@mattisonchao
Remove unused import
61c81d9
@Technoboy- Technoboy- removed the release/blocker Indicate the PR or issue that should block the release until it gets resolved label Aug 4, 2022
@Technoboy- Technoboy- merged commit b395ad4 into apache:master Aug 4, 2022
@mattisonchao mattisonchao deleted the revert_14672 branch August 4, 2022 07:04
@github-actions
Copy link

github-actions bot commented Aug 4, 2022

@mattisonchao Please provide a correct documentation label for your PR.
Instructions see Pulsar Documentation Label Guide.

Technoboy- pushed a commit to merlimat/pulsar that referenced this pull request Aug 16, 2022
@mattisonchao @Technoboy-
Revert "Change rollCurrentLedgerIfFull logic to follow lazy creat…
a47464b 
…ion of ledger (apache#14672) (apache#16806)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Technoboy- Technoboy- Technoboy- approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@BewareMyPower BewareMyPower BewareMyPower approved these changes

Assignees

@mattisonchao mattisonchao

Labels

doc-label-missing

Projects

None yet

Milestone

2.11.0

Development

Successfully merging this pull request may close these issues.

6 participants

@mattisonchao @BewareMyPower @Nicklee007 @Technoboy- @codelipenghui @poorbarcode