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/01/15 12:13:52 UTC
[GitHub] [pulsar] ltamber opened a new pull request #9219: support unackMessages stats for cumulative ack
ltamber opened a new pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219
### Motivation
Currently, the unackMessages was always 0 in cumulativeAck mode if show topic stats, this pull request calculated unacked messages both in cumulative ack and individual ack mode.
----------------------------------------------------------------
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] sijie commented on a change in pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#discussion_r559320595
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java
##########
@@ -169,12 +170,7 @@ public Consumer(Subscription subscription, SubType subType, String topicName, lo
stats.setClientVersion(cnx.getClientVersion());
stats.metadata = this.metadata;
- if (Subscription.isIndividualAckMode(subType)) {
- this.pendingAcks = new ConcurrentLongLongPairHashMap(256, 1);
- } else {
- // We don't need to keep track of pending acks if the subscription is not shared
- this.pendingAcks = null;
- }
+ this.pendingAcks = new ConcurrentLongLongPairHashMap(256, 1);
}
Review comment:
Can we avoid creating a hashmap if we don't need it?
----------------------------------------------------------------
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] ltamber commented on a change in pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
ltamber commented on a change in pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#discussion_r559345248
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java
##########
@@ -169,12 +170,7 @@ public Consumer(Subscription subscription, SubType subType, String topicName, lo
stats.setClientVersion(cnx.getClientVersion());
stats.metadata = this.metadata;
- if (Subscription.isIndividualAckMode(subType)) {
- this.pendingAcks = new ConcurrentLongLongPairHashMap(256, 1);
- } else {
- // We don't need to keep track of pending acks if the subscription is not shared
- this.pendingAcks = null;
- }
+ this.pendingAcks = new ConcurrentLongLongPairHashMap(256, 1);
}
Review comment:
we need to track of pending acks in all subscription mode, I thinks this is also need to create a hashmap
----------------------------------------------------------------
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] zymap commented on pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#issuecomment-784193843
Move this to the next release.
----------------------------------------------------------------
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] Anonymitaet commented on pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#issuecomment-844905096
Hi @ltamber thanks for your code work. For this PR, do we need to update docs?
--
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] ltamber commented on pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
ltamber commented on pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#issuecomment-764622732
/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 #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#issuecomment-871241456
@ltamber hi, I am working on release pulsar 2.7.3, could you please solve the conflict and fix the comment? thanks.
--
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] ltamber closed pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
ltamber closed pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219
--
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] michaeljmarshall commented on pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#issuecomment-991183171
@ltamber - do you plan on completing the work for this PR? There are conflicts right now. As such, I am going to remove the 2.7.4 label. I added the 2.10 milestone. If/when we merge this, we can select the branches to cherry-pick to.
--
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] ltamber commented on pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
ltamber commented on pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#issuecomment-764628898
/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] sijie commented on a change in pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#discussion_r574239559
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java
##########
@@ -169,12 +170,7 @@ public Consumer(Subscription subscription, SubType subType, String topicName, lo
stats.setClientVersion(cnx.getClientVersion());
stats.metadata = this.metadata;
- if (Subscription.isIndividualAckMode(subType)) {
- this.pendingAcks = new ConcurrentLongLongPairHashMap(256, 1);
- } else {
- // We don't need to keep track of pending acks if the subscription is not shared
- this.pendingAcks = null;
- }
+ this.pendingAcks = new ConcurrentLongLongPairHashMap(256, 1);
}
Review comment:
pending acks are only used when the transaction is enabled.
----------------------------------------------------------------
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] ltamber commented on pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
ltamber commented on pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#issuecomment-991823800
> @ltamber - do you plan on completing the work for this PR? There are conflicts right now. As such, I am going to remove the 2.7.4 label. I added the 2.10 milestone. If/when we merge this, we can select the branches to cherry-pick to.
sorry for that, I'll close this PR first.
--
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] ltamber closed pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
ltamber closed pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219
--
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] ltamber commented on pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
ltamber commented on pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#issuecomment-764622732
----------------------------------------------------------------
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] ltamber commented on pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
ltamber commented on pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#issuecomment-991823800
> @ltamber - do you plan on completing the work for this PR? There are conflicts right now. As such, I am going to remove the 2.7.4 label. I added the 2.10 milestone. If/when we merge this, we can select the branches to cherry-pick to.
sorry for that, I'll close this PR first.
--
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] michaeljmarshall commented on pull request #9219: support unackMessages stats for cumulative ack
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #9219:
URL: https://github.com/apache/pulsar/pull/9219#issuecomment-1004405306
@ltamber - no worries. Do you know if this issue still exists? If it does, maybe we can create a GitHub issue to track and hopefully resolve the bug. Thanks.
--
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