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/10/05 18:24:09 UTC
[GitHub] [pulsar] fmiguelez opened a new pull request #8205: [Issue 8204][bookkeeper] Estimate backlog size as total if no consumer is present
fmiguelez opened a new pull request #8205:
URL: https://github.com/apache/pulsar/pull/8205
Fixes #8204
### Motivation
The origin of the BUG was found in ManagedLedgerImpl.getEstimatedBacklogSize() returning 0 if no subscription was found. Instead of returning 0 we return the total size of the ledger.
### Modifications
Return total size of ledger instead of 0 if no subscription is present (the case for reader or non-durable subscriptions).
### Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
### Does this pull request potentially affect one of the following parts:
- 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: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
- Does this pull request introduce a new feature? (no)
- If yes, how is the feature documented? (not applicable)
- If a feature is not applicable for documentation, explain why? (A very internal feature)
----------------------------------------------------------------
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] fmiguelez commented on pull request #8205: [Issue 8204][bookkeeper] Estimate backlog size as total if no consumer is present
Posted by GitBox <gi...@apache.org>.
fmiguelez commented on pull request #8205:
URL: https://github.com/apache/pulsar/pull/8205#issuecomment-704068552
Hi @sijie thanks for your comments.
Well if a ledger has no subscriptions but has alredy messages published to it you suggest that its size is zero but from the first moment someone connects (a subscription is created) then zero goes to whatever size ledger has. But if there is at least one subscription that has not consumed anything yet then it is ok to return the total size of the ledger even if rest of subscriptions have already consumed all of it.
I do not see that "bad" returning the total size of ledger if there are no durable subscriptions at all (take into consideration that `NonDurable` subscriptions do not count in this case) instead of zero size if there is actually data published to it. Indeed that is the backlog size all subscriptions will have that start out fresh from that moment on.
In any case if you do not feel confortable with the change or you think it could break other things it is perfectly fine for me but I think that original issue #8204 must be tackled other way then.
The main problem is that if a topic receives data but no persistent subscription has been registered yet because consumers are still starting/stopped or because those consumers only use `NonDurable` subscriptions (such as with the `Reader` interface) automatic compaction will never be triggered. However if we trigger it manually it does work as expected. There is obviously a BUG there. The troubled logic is inside `PersistentTopic.checkCompaction()` method:
```
if (compactionSub != null) {
backlogEstimate = compactionSub.estimateBacklogSize();
} else {
// compaction has never run, so take full backlog size
// We enter here because "compaction" sub does not exist unless
// it has manually been triggered. This returns 0 as no other "durable"
// subscriptions are available but ledger does has data
backlogEstimate = ledger.getEstimatedBacklogSize();
}
// compactionThreshold must be configured over 0 si this can work
// but backlogExtimate will always be 0
if (backlogEstimate > compactionThreshold) {
}
```
----------------------------------------------------------------
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] fmiguelez edited a comment on pull request #8205: [Issue 8204][bookkeeper] Estimate backlog size as total if no consumer is present
Posted by GitBox <gi...@apache.org>.
fmiguelez edited a comment on pull request #8205:
URL: https://github.com/apache/pulsar/pull/8205#issuecomment-704133918
> @fmiguelez - Just want to be very clear on storage size and backlog. The storage size is the total amount of bytes in the topic. This is already exposed as part of topic stats. The backlog is a concept for measuring how many messages are not consumed by a subscription. The backlog conception is a concept of a subscription, not a topic. I recorded a session walking through those conceptions. You can watch it at https://www.youtube.com/watch?v=PIX570nyq_c&list=PLqRma1oIkcWhWAhKgImEeRiQi5vMlqTc-&index=4&t=1368s and view the slides at https://www.slideshare.net/streamnative/tgi-pulsar-episode-016-backlog-vs-storage-size.
>
> If the intention is to fix #8204, we should just change the logic to use storage size for triggering topic compaction instead of using backlog size.
Looks good to me.
@sijie I am closing this pull request and opening a new one with the changes suggested. It think it is interesting to keep this conversation as reference instead of simple remove the pull request.
----------------------------------------------------------------
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 pull request #8205: [Issue 8204][bookkeeper] Estimate backlog size as total if no consumer is present
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8205:
URL: https://github.com/apache/pulsar/pull/8205#issuecomment-704102947
@fmiguelez - Just want to be very clear on storage size and backlog. The storage size is the total amount of bytes in the topic. This is already exposed as part of topic stats. The backlog is a concept for measuring how many messages are not consumed by a subscription. The backlog conception is a concept of a subscription, not a topic. I recorded a session walking through those conceptions. You can watch it at https://www.youtube.com/watch?v=PIX570nyq_c&list=PLqRma1oIkcWhWAhKgImEeRiQi5vMlqTc-&index=4&t=1368s and view the slides at https://www.slideshare.net/streamnative/tgi-pulsar-episode-016-backlog-vs-storage-size.
If the intention is to fix #8204, we should just change the logic to use storage size for triggering topic compaction instead of using backlog 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] fmiguelez commented on pull request #8205: [Issue 8204][bookkeeper] Estimate backlog size as total if no consumer is present
Posted by GitBox <gi...@apache.org>.
fmiguelez commented on pull request #8205:
URL: https://github.com/apache/pulsar/pull/8205#issuecomment-704144955
Original issue will be addressed in other pull request.
----------------------------------------------------------------
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] fmiguelez edited a comment on pull request #8205: [Issue 8204][bookkeeper] Estimate backlog size as total if no consumer is present
Posted by GitBox <gi...@apache.org>.
fmiguelez edited a comment on pull request #8205:
URL: https://github.com/apache/pulsar/pull/8205#issuecomment-704133918
> @fmiguelez - Just want to be very clear on storage size and backlog. The storage size is the total amount of bytes in the topic. This is already exposed as part of topic stats. The backlog is a concept for measuring how many messages are not consumed by a subscription. The backlog conception is a concept of a subscription, not a topic. I recorded a session walking through those conceptions. You can watch it at https://www.youtube.com/watch?v=PIX570nyq_c&list=PLqRma1oIkcWhWAhKgImEeRiQi5vMlqTc-&index=4&t=1368s and view the slides at https://www.slideshare.net/streamnative/tgi-pulsar-episode-016-backlog-vs-storage-size.
>
> If the intention is to fix #8204, we should just change the logic to use storage size for triggering topic compaction instead of using backlog size.
Looks good to me.
@sijie I am closing this pull request and opening a new one with the changes suggested. It think it is interesting to keep this conversation as reference instead of simple remove the pull request. Follow-up pull-request is #8209.
----------------------------------------------------------------
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] fmiguelez commented on pull request #8205: [Issue 8204][bookkeeper] Estimate backlog size as total if no consumer is present
Posted by GitBox <gi...@apache.org>.
fmiguelez commented on pull request #8205:
URL: https://github.com/apache/pulsar/pull/8205#issuecomment-704133918
> @fmiguelez - Just want to be very clear on storage size and backlog. The storage size is the total amount of bytes in the topic. This is already exposed as part of topic stats. The backlog is a concept for measuring how many messages are not consumed by a subscription. The backlog conception is a concept of a subscription, not a topic. I recorded a session walking through those conceptions. You can watch it at https://www.youtube.com/watch?v=PIX570nyq_c&list=PLqRma1oIkcWhWAhKgImEeRiQi5vMlqTc-&index=4&t=1368s and view the slides at https://www.slideshare.net/streamnative/tgi-pulsar-episode-016-backlog-vs-storage-size.
>
> If the intention is to fix #8204, we should just change the logic to use storage size for triggering topic compaction instead of using backlog size.
Looks good to me.
----------------------------------------------------------------
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] fmiguelez closed pull request #8205: [Issue 8204][bookkeeper] Estimate backlog size as total if no consumer is present
Posted by GitBox <gi...@apache.org>.
fmiguelez closed pull request #8205:
URL: https://github.com/apache/pulsar/pull/8205
----------------------------------------------------------------
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