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.

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

[improve][txn] PIP-160: Transaction log store enables the batch feature #16685

Merged
merged 1 commit into from
Jul 22, 2022
Merged

[improve][txn] PIP-160: Transaction log store enables the batch feature #16685

merged 1 commit into from
Jul 22, 2022

Conversation

poorbarcode
Copy link
Contributor

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.

Documentation

  • doc-required

  • doc-not-needed

  • doc

  • doc-complete

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 19, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 20, 2022
@codelipenghui codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/transaction labels Jul 20, 2022
@codelipenghui codelipenghui changed the title [improve] [txn] [PIP-160] Transaction log store enables the batch feature [improve] [txn] PIP-160: Transaction log store enables the batch feature Jul 20, 2022
@codelipenghui codelipenghui changed the title [improve] [txn] PIP-160: Transaction log store enables the batch feature [improve][txn] PIP-160: Transaction log store enables the batch feature Jul 20, 2022
@poorbarcode poorbarcode requested a review from congbobo184 July 20, 2022 13:41
Technoboy-
Technoboy- reviewed Jul 21, 2022
View reviewed changes
pulsar-broker/src/test/java/org/apache/pulsar/broker/stats/ManagedLedgerMetricsTest.java Outdated
MLTransactionLogImpl mlTransactionLog = new MLTransactionLogImpl(TransactionCoordinatorID.get(0),
pulsar.getManagedLedgerFactory(), managedLedgerConfig, txnLogBufferedWriterConfig,
scheduledExecutorService);
mlTransactionLog.initialize().join();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to add a timeout on the method or here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Already fixed

congbobo184
congbobo184 reviewed Jul 21, 2022
View reviewed changes
.../src/test/java/org/apache/pulsar/transaction/coordinator/MLTransactionMetadataStoreTest.java
@@ -55,12 +60,19 @@

public class MLTransactionMetadataStoreTest extends MockedBookKeeperTestCase {

private ScheduledExecutorService scheduledExecutorService = Executors.newScheduledThreadPool(3);
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. It's better to add a single test that adds batch transaction log and then can open a new cursor for the transaction log managedLedger and read the batch log. then the TransactionCoordinator can recover successfully
  2. add a test for the log delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already fixed, it is MLTransactionLogImplTest

codelipenghui
codelipenghui reviewed Jul 21, 2022
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated Show resolved Hide resolved
...nator/src/main/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionLogImpl.java Outdated Show resolved Hide resolved
...nator/src/main/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionLogImpl.java Show resolved Hide resolved
...nator/src/main/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionLogImpl.java Show resolved Hide resolved
...nator/src/main/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionLogImpl.java Show resolved Hide resolved
...nator/src/main/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionLogImpl.java Outdated Show resolved Hide resolved
...tor/src/main/java/org/apache/pulsar/transaction/coordinator/impl/TxnBatchedPositionImpl.java Outdated Show resolved Hide resolved
codelipenghui
codelipenghui reviewed Jul 21, 2022
View reviewed changes
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.

Looks good to me, just left some minor comments.

...src/test/java/org/apache/pulsar/transaction/coordinator/impl/TxnBatchedPositionImplTest.java Outdated Show resolved Hide resolved
...r/src/test/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionLogImplTest.java Outdated Show resolved Hide resolved
...r/src/test/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionLogImplTest.java Outdated Show resolved Hide resolved
...r/src/test/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionLogImplTest.java Outdated Show resolved Hide resolved
...r/src/test/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionLogImplTest.java Show resolved Hide resolved
...r/src/test/java/org/apache/pulsar/transaction/coordinator/impl/MLTransactionLogImplTest.java Show resolved Hide resolved
@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

@congbobo184 congbobo184 merged commit 4d16ad5 into apache:master Jul 22, 2022
@poorbarcode poorbarcode deleted the pip/160-7 branch September 17, 2022 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@congbobo184 congbobo184 congbobo184 approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@gaoran10 gaoran10 Awaiting requested review from gaoran10

@liangyepianzhou liangyepianzhou Awaiting requested review from liangyepianzhou

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

Assignees

@poorbarcode poorbarcode

Labels
area/transaction doc-not-needed Your PR changes do not impact docs type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Milestone
2.11.0
Development

Successfully merging this pull request may close these issues.
5 participants
@poorbarcode @Technoboy- @codelipenghui @congbobo184 @liangyepianzhou