Skip to content

Conversation

@poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Jul 22, 2022

Master Issue: #15370

Motivation

see #15370

Modifications

I will complete proposal #15370 with these pull requests( current pull request is a part of step-6 ):

  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 ).
    GET /admin/v3/transactions/coordinatorStats
    GET /admin/v3/transactions/pendingAckStats/:tenant/:namespace:/:topic:/:subName
  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 22, 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, please check the failed test.

@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 23, 2022
@codelipenghui codelipenghui added area/transaction component/stats type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages labels Jul 23, 2022
/**
* If enabled the feature-batch, this attribute means maximum wait time(in millis) for the first record in a batch.
*/
private int batchedWriteMaxDelayInMillis;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please help check the naming of the above fields @momo-jun

Copy link
Contributor

Choose a reason for hiding this comment

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

I made a few suggestions for the annotation.

Copy link
Contributor

@liangyepianzhou liangyepianzhou left a comment

Choose a reason for hiding this comment

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

I do not understand why the users need the admin API to get the configuration of the transaction batch. The configuration is what the user configures in broker.conf, isn't it?

/** Whether to enable the batch feature when writing to Bookie. **/
private boolean batchEnabled = false;

/** If enabled the feature-batch, this attribute means maximum log records count in a batch. **/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** If enabled the feature-batch, this attribute means maximum log records count in a batch. **/
/** If the batch feature is enabled, this attribute means the maximum count of log records in a batch. **/

private int batchedWriteMaxSize;

/**
* If enabled the feature-batch, this attribute means maximum wait time(in millis) for the first record in a batch.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If enabled the feature-batch, this attribute means maximum wait time(in millis) for the first record in a batch.
* If the batch feature is enabled, this attribute means the maximum wait time (in milliseconds) for the first record in a batch.

/** If enabled the feature-batch, this attribute means maximum log records count in a batch. **/
private int batchedWriteMaxRecords = 512;

/** If enabled the feature-batch, this attribute means bytes size in a batch. **/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** If enabled the feature-batch, this attribute means bytes size in a batch. **/
/** If the batch feature is enabled, this attribute means the maximum size of bytes in a batch. **/

@poorbarcode poorbarcode marked this pull request as draft July 25, 2022 02:30
@poorbarcode
Copy link
Contributor Author

I do not understand why the users need the admin API to get the configuration of the transaction batch. The configuration is what the user configures in broker.conf, isn't it?

OK, I have make this PR to "draft"

@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@poorbarcode poorbarcode closed this Aug 7, 2022
@poorbarcode poorbarcode deleted the pip/160-13 branch September 17, 2022 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

5 participants