-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
[improve][txn] PIP-160: Pending ack log store enables the batch feature #16707
Conversation
3630888
to
24375a5
Compare
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/MLPendingAckStore.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/MLPendingAckStore.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/MLPendingAckStore.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/org/apache/pulsar/broker/transaction/pendingack/impl/MLPendingAckStore.java
Outdated
Show resolved
Hide resolved
/pulsarbot run-failure-checks |
3 similar comments
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
@poorbarcode #16727 has merged, could you please use the shared time for this PR? |
Already fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left some minor comments.
@Getter | ||
private final ScheduledExecutorService transactionTimer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks unused.
this.transactionTimer = Executors | ||
.newSingleThreadScheduledExecutor(new DefaultThreadFactory("pulsar-backlog-quota-checker")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already remove. Thanks.
buffer.resetReaderIndex(); | ||
if (magicNum == BATCHED_ENTRY_DATA_PREFIX_MAGIC_NUMBER){ | ||
// skip version | ||
buffer.skipBytes(4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already fixed. Thanks.
} | ||
}).when(pendingAckHandle).completeHandleFuture(); | ||
mlPendingAckStoreForRead.replayAsync(pendingAckHandle, internalPinnedExecutor); | ||
Awaitility.await().atMost(200, TimeUnit.SECONDS).until(() -> processController.get() == 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why need to wait at most 200 seconds? we don't have too many logs in the pending ack store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already change to 2 seconds.
/pulsarbot rerun-failure-checks |
1 similar comment
/pulsarbot rerun-failure-checks |
…re (apache#16707) Master Issue: apache#15370 ### Motivation see apache#15370 ### Modifications I will complete proposal apache#15370 with these pull requests( *current pull request is the step-4* ): 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.
Master Issue: #15370
Motivation
see #15370
Modifications
I will complete proposal #15370 with these pull requests( current pull request is the step-4 ):
TxnLogBufferedWriter
Documentation
doc-required
doc-not-needed
doc
doc-complete