You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/07/21 07:43:14 UTC
[GitHub] [pulsar] lhotari opened a new pull request #11407: [Transaction] Fix direct memory leak related to commit and abort markers
lhotari opened a new pull request #11407:
URL: https://github.com/apache/pulsar/pull/11407
### Motivation
- Netty ByteBufs for commit and abort markers currently leak direct memory
- `ManagedLedgerImpl.asyncAddEntry` will increase the reference count by calling `.retain()` for the marker message's ByteBuf (in `OpAddEntry.create`) and therefore the reference count has to be decreased with the call to `.release()` after the `asyncAddEntry` call.
### Modifications
- call `.release()` in try-finally block for commit and abort markers after the `asyncAddEntry` call
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #11407: [Transaction] Fix direct memory leak related to commit and abort markers
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #11407:
URL: https://github.com/apache/pulsar/pull/11407#issuecomment-885337974
@lhotari I have removed the `release/2.7.4` tag for this PR since for branch-2.7, the transaction is just a developer preview version, some features does not complete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari closed pull request #11407: [Transaction] Fix direct memory leak related to commit and abort markers
Posted by GitBox <gi...@apache.org>.
lhotari closed pull request #11407:
URL: https://github.com/apache/pulsar/pull/11407
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #11407: [Transaction] Fix direct memory leak related to commit and abort markers
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #11407:
URL: https://github.com/apache/pulsar/pull/11407#discussion_r673752495
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java
##########
@@ -206,24 +206,28 @@ private void handleTransactionMessage(TxnID txnId, Position position) {
ByteBuf commitMarker = Markers.newTxnCommitMarker(-1L, txnID.getMostSigBits(),
txnID.getLeastSigBits());
- topic.getManagedLedger().asyncAddEntry(commitMarker, new AsyncCallbacks.AddEntryCallback() {
- @Override
- public void addComplete(Position position, ByteBuf entryData, Object ctx) {
- synchronized (TopicTransactionBuffer.this) {
- updateMaxReadPosition(txnID);
- handleLowWaterMark(txnID, lowWaterMark);
- clearAbortedTransactions();
- takeSnapshotByChangeTimes();
+ try {
+ topic.getManagedLedger().asyncAddEntry(commitMarker, new AsyncCallbacks.AddEntryCallback() {
Review comment:
in this thread.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #11407: [Transaction] Fix direct memory leak related to commit and abort markers
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #11407:
URL: https://github.com/apache/pulsar/pull/11407#discussion_r673748945
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java
##########
@@ -206,24 +206,28 @@ private void handleTransactionMessage(TxnID txnId, Position position) {
ByteBuf commitMarker = Markers.newTxnCommitMarker(-1L, txnID.getMostSigBits(),
txnID.getLeastSigBits());
- topic.getManagedLedger().asyncAddEntry(commitMarker, new AsyncCallbacks.AddEntryCallback() {
- @Override
- public void addComplete(Position position, ByteBuf entryData, Object ctx) {
- synchronized (TopicTransactionBuffer.this) {
- updateMaxReadPosition(txnID);
- handleLowWaterMark(txnID, lowWaterMark);
- clearAbortedTransactions();
- takeSnapshotByChangeTimes();
+ try {
+ topic.getManagedLedger().asyncAddEntry(commitMarker, new AsyncCallbacks.AddEntryCallback() {
Review comment:
should we release commitMarker in this thread or inside the callback ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari merged pull request #11407: [Transaction] Fix direct memory leak related to commit and abort markers
Posted by GitBox <gi...@apache.org>.
lhotari merged pull request #11407:
URL: https://github.com/apache/pulsar/pull/11407
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org