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 2020/11/18 13:04:05 UTC
[GitHub] [pulsar] congbobo184 opened a new pull request #8612: [Transaction] Add the batch size in transaction ack command.
congbobo184 opened a new pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612
## Motivation
Now in Failover sub we ack with transaction will not get the batch size from consumer pendingAcks, so we should ack request carry the batch size.
## implement
We ack with transaction will carry the bath size for individual ack delete the consumer pendingAcks.
### Verifying this change
Add the tests for it
Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes
Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (no)
The default values of configurations: (no)
The wire protocol: (yes)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612#issuecomment-730826418
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 closed pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
congbobo184 closed pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612#issuecomment-730776145
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612#issuecomment-730782211
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612#issuecomment-730296246
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 removed a comment on pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
congbobo184 removed a comment on pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612#issuecomment-730776145
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612#discussion_r526092501
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java
##########
@@ -391,7 +391,7 @@ public void readEntryFailed(ManagedLedgerException exception, Object ctx) {
List<MutablePair<PositionImpl, Long>> positions) {
if (pendingAckHandle == null) {
return FutureUtil.failedFuture(
- new TransactionConflictException("Broker does't support Transaction pending ack!"));
+ new NotAllowedException("Broker does't support Transaction pending ack!"));
Review comment:
```suggestion
new NotAllowedException("The transaction is disabled"));
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a change in pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612#discussion_r526092501
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentSubscription.java
##########
@@ -391,7 +391,7 @@ public void readEntryFailed(ManagedLedgerException exception, Object ctx) {
List<MutablePair<PositionImpl, Long>> positions) {
if (pendingAckHandle == null) {
return FutureUtil.failedFuture(
- new TransactionConflictException("Broker does't support Transaction pending ack!"));
+ new NotAllowedException("Broker does't support Transaction pending ack!"));
Review comment:
```suggestion
new NotAllowedException("The transaction is not enabled"));
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java
##########
@@ -435,9 +435,6 @@ void doUnsubscribe(final long requestId) {
private CompletableFuture<Void> individualAckNormal(CommandAck ack, Map<String,Long> properties) {
List<Position> positionsAcked = new ArrayList<>();
List<PositionImpl> checkBatchPositions = null;
Review comment:
This line can be deleted?
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ConsumerImpl.java
##########
@@ -2416,15 +2418,16 @@ private void removeChunkMessage(String msgUUID, ChunkedMessageCtx chunkedMsgCtx,
bitSetRecyclable.set(0, batchMessageId.getBatchSize());
bitSetRecyclable.clear(batchMessageId.getBatchIndex());
}
+ cmd = Commands.newAck(consumerId, ledgerId, entryId, bitSetRecyclable, ackType, validationError, properties,
+ txnID.getLeastSigBits(), txnID.getMostSigBits(), requestId, batchMessageId.getBatchSize());
Review comment:
Only the txn is not null needs to set the batch size.
##########
File path: pulsar-common/src/main/proto/PulsarApi.proto
##########
@@ -60,6 +60,7 @@ message MessageIdData {
optional int32 partition = 3 [default = -1];
optional int32 batch_index = 4 [default = -1];
repeated int64 ack_set = 5;
+ optional uint64 batch_size = 6;
Review comment:
```suggestion
optional int32 batch_size = 6;
```
int32 is enough for batch size.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 removed a comment on pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
congbobo184 removed a comment on pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612#issuecomment-730778931
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612#issuecomment-730346086
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] congbobo184 commented on pull request #8612: [Transaction] Add the batch size in transaction ack command.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #8612:
URL: https://github.com/apache/pulsar/pull/8612#issuecomment-730778931
/pulsarbot run-failure-checks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org