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] Txn buffered writer for transaction log batch #16428

Merged
merged 36 commits into from
Jul 19, 2022
Merged

[improve] [txn] [PIP-160] Txn buffered writer for transaction log batch #16428

merged 36 commits into from
Jul 19, 2022

Conversation

poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Jul 7, 2022 •
edited
Loading

Master Issue: #15370

Motivation

see #15370

Modifications

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

  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 7, 2022
@poorbarcode
Copy link
Contributor Author

@liangyepianzhou @congbobo184 Could you take a look

@poorbarcode poorbarcode changed the title [improve][client] [PIP-165] Make transactions work more efficiently by aggregation operation for transaction log and pending ack store [improve] [transaction] [PIP-165] Make transactions work more efficiently by aggregation operation for transaction log and pending ack store Jul 7, 2022
@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

@poorbarcode poorbarcode changed the title [improve] [transaction] [PIP-165] Make transactions work more efficiently by aggregation operation for transaction log and pending ack store [improve] [transaction] [PIP-160] Make transactions work more efficiently by aggregation operation for transaction log and pending ack store Jul 7, 2022
@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

1 similar comment
@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

congbobo184
congbobo184 reviewed Jul 11, 2022
View reviewed changes
...nator/src/main/java/org/apache/pulsar/transaction/coordinator/impl/TxnLogBufferedWriter.java Outdated
prefix.writeChar(BATCHED_ENTRY_DATA_PREFIX_VERSION);
ByteBuf actualContent = this.dataSerializer.serialize(this.dataArray);
ByteBuf pairByteBuf = Unpooled.wrappedUnmodifiableBuffer(prefix, actualContent);
FlushContext<T> flushContext = FlushContext.newInstance(this.dataArray, this.asyncAddArgsList);
Copy link
Contributor

Choose a reason for hiding this comment

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

why FlushContext should use dataArray

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addComplete(Position position, ByteBuf byteBuf, Object ctx) we return null param-byteBuf. Maybe callback implementation needs the data(T) in the future.

Does not need it right now, so delete it.

@poorbarcode poorbarcode requested a review from congbobo184 July 11, 2022 03:12
@congbobo184 congbobo184 changed the title [improve] [transaction] [PIP-160] Make transactions work more efficiently by aggregation operation for transaction log and pending ack store [improve] [txn] [PIP-160] Make transactions work more efficiently by aggregation operation for transaction log and pending ack store Jul 11, 2022
@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 12, 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 12, 2022
congbobo184
congbobo184 reviewed Jul 12, 2022
View reviewed changes
...r/src/test/java/org/apache/pulsar/transaction/coordinator/impl/TxnLogBufferedWriterTest.java Outdated
byteBufBatchedEntryDataList.add(content);
return content;
}
}, 512, 1024 * 1024 * 4, 1, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

every condition needs to be tested. In this case are time, data number, data size

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 suggestion

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it's good to have a data provider.

@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

@poorbarcode poorbarcode changed the title [improve] [txn] [PIP-160] Make transactions work more efficiently by aggregation operation for transaction log and pending ack store [improve] [txn] [PIP-160] TXN buffered writer for transaction log batch Jul 15, 2022
@poorbarcode poorbarcode changed the title [improve] [txn] [PIP-160] TXN buffered writer for transaction log batch [improve] [txn] [PIP-160] Txn buffered writer for transaction log batch Jul 15, 2022
codelipenghui
codelipenghui approved these changes Jul 15, 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 general good me, left some minor comments.

...nator/src/main/java/org/apache/pulsar/transaction/coordinator/impl/TxnLogBufferedWriter.java Outdated
Comment on lines 349 to 351
log.error("Cancel task that schedule at fixed rate trig flush failure. The state will stay at CLOSING."
+ " managedLedger: " + managedLedger.getName());
});
Copy link
Contributor

Choose a reason for hiding this comment

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

If the scheduledFuture already cancelled, we don't need this log.

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.

@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

2 similar comments
@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

@poorbarcode
Copy link
Contributor Author

/pulsarbot run-failure-checks

@codelipenghui codelipenghui merged commit af932ed into apache:master Jul 19, 2022
@poorbarcode poorbarcode deleted the pip/160 branch July 19, 2022 01:54
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

@liangyepianzhou liangyepianzhou liangyepianzhou approved these changes

@gaoran10 gaoran10 Awaiting requested review from gaoran10

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.
4 participants
@poorbarcode @codelipenghui @congbobo184 @liangyepianzhou