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/26 16:56:24 UTC
[GitHub] [pulsar] eolivelli opened a new pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic (due to message retention)
eolivelli opened a new pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725
This is only a reproducer for issue #8677
The test case reproduces this case:
- you restart a broker
- a new ledger is appended to the list of active ledgers for the ManagerLedger
- the previous ledger is trimmed (because it is expired and it is not the 'currentLedger')
- the last message id still points to the trimmed ledger
in this case calling getLastMessageId fails
probably the problem is that the ledger is trimmed even if it referred by the last message id
----------------------------------------------------------------
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] eolivelli commented on pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#issuecomment-734696784
@merlimat @codelipenghui @rdhabalia @sijie PTAL
Is it too late to pick this fix into 2.7.0 and 2.6.3 ?
----------------------------------------------------------------
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] eolivelli commented on a change in pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#discussion_r532411494
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2089,6 +2089,10 @@ void internalTrimConsumedLedgers(CompletableFuture<?> promise) {
log.debug("[{}] Ledger {} skipped for deletion as it is currently being written to", name,
ls.getLedgerId());
break;
+ } else if (currentLastConfirmedEntry != null && ls.getLedgerId() == currentLastConfirmedEntry.getLedgerId()) {
Review comment:
in the case of the user it would be better to have this messageId working as in this patch, although we have strong rules about retention rules due to privacy laws (but you can always have a technical motivation for keeping the data for a while).
----------------------------------------------------------------
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 #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#discussion_r537497615
##########
File path: .gitignore
##########
@@ -30,6 +30,9 @@ pulsar-functions/worker/src/test/resources/
*.iml
*.iws
+# NetBeans
+nb-configuration.xml
+
Review comment:
Ok, I see.
----------------------------------------------------------------
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 #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#discussion_r537337219
##########
File path: .gitignore
##########
@@ -30,6 +30,9 @@ pulsar-functions/worker/src/test/resources/
*.iml
*.iws
+# NetBeans
+nb-configuration.xml
+
Review comment:
The change 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] eolivelli edited a comment on pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
eolivelli edited a comment on pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#issuecomment-734696784
@merlimat @codelipenghui @rdhabalia @sijie PTAL
Is it too late to pick this fix into 2.7.1 and 2.6.3 ?
----------------------------------------------------------------
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] eolivelli commented on pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#issuecomment-737739387
ping @sijie @rdhabalia
----------------------------------------------------------------
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 #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#issuecomment-738623149
@codelipenghui Can you take a look?
----------------------------------------------------------------
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] eolivelli commented on pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#issuecomment-736253709
/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] eolivelli commented on a change in pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#discussion_r532406502
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2089,6 +2089,10 @@ void internalTrimConsumedLedgers(CompletableFuture<?> promise) {
log.debug("[{}] Ledger {} skipped for deletion as it is currently being written to", name,
ls.getLedgerId());
break;
+ } else if (currentLastConfirmedEntry != null && ls.getLedgerId() == currentLastConfirmedEntry.getLedgerId()) {
Review comment:
@sijie thanks for checking
Yes, I don't know what is the expected value for "lastMessageId" in this case.
It is true that this is a behaviour change in the rollover but my rationale is that if we send an "empty" messageId, the consumer (or the "Reader" in the case of the user that reported the issue) won't see a consistent sequence of "lastMessageId"
You can have:
- time 1: last messageId: X:Y:Z:K
- time 2 (broker restart): messageId: empty
- time 3 (after one write): messageId: X+1:Y:Z:K
so if you do not restart the broker and everything goes well you do not see that hole in the sequence
In the case of the user the lastMessageId is used to compute an approximate number of messages to be consumed by the Reader. as the topic is basically "empty" the effect is the same, that is: there are no more messages to consume. So your suggestion would make the user happy anyhow.
Regarding the trim: we are slightly changing the semantics, but as soon as you write a new message, the behavior is the same. Currently (AFAIK but I still have limited knowledge so I may be wrong) there is not strict contract about when a ledger is deleted (and also you won't know when it will be really deleted from the bookies).
All in all I would prefer this new behaviour:
- the sequence of lastMessageId does not contain "holes"
- the trim behavior changes but is it already unpredictable
You point "data won't ever be deleted if there is no more write to the topic" is valid, but I am not sure how much impact could have on production users. Is it about privacy laws/data retention rules ?
but I am open to change the patch
----------------------------------------------------------------
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 merged pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725
----------------------------------------------------------------
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] eolivelli commented on pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#issuecomment-735900936
@sijie I have updated the patch, waiting for CI
----------------------------------------------------------------
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] eolivelli commented on a change in pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#discussion_r532706531
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2089,6 +2089,10 @@ void internalTrimConsumedLedgers(CompletableFuture<?> promise) {
log.debug("[{}] Ledger {} skipped for deletion as it is currently being written to", name,
ls.getLedgerId());
break;
+ } else if (currentLastConfirmedEntry != null && ls.getLedgerId() == currentLastConfirmedEntry.getLedgerId()) {
Review comment:
@sijie I will adapt the patch as you are requiring.
Thanks for your feedback
----------------------------------------------------------------
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] eolivelli commented on a change in pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#discussion_r537370632
##########
File path: .gitignore
##########
@@ -30,6 +30,9 @@ pulsar-functions/worker/src/test/resources/
*.iml
*.iws
+# NetBeans
+nb-configuration.xml
+
Review comment:
@codelipenghui you are right.
I can send an additional patch if you prefer, this addition to .gitignore is very helpful for Apache NetBeans users and I always have to ignore those files.
Shall I remove 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] sijie commented on a change in pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#discussion_r532705475
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2089,6 +2089,10 @@ void internalTrimConsumedLedgers(CompletableFuture<?> promise) {
log.debug("[{}] Ledger {} skipped for deletion as it is currently being written to", name,
ls.getLedgerId());
break;
+ } else if (currentLastConfirmedEntry != null && ls.getLedgerId() == currentLastConfirmedEntry.getLedgerId()) {
Review comment:
> but I am not sure how much impact could have on production users. Is it about privacy laws/data retention rules ?
In a lot of Pulsar user cases, there are topics with relatively low traffic. If there are no writes going to the topic, it will cause accumulating one segment per topic. If there are a lot of such topics, it will cause disk filling up quickly. So I don't think we should change the retention behavior. This is a -1 from me.
As I suggested, we should fix `getLastMessageId` behavior. Because there is one empty message-id. We can still return a messageId with `entryId == -1`.
----------------------------------------------------------------
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] eolivelli commented on pull request #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#issuecomment-737739636
At MagNews.com we are on 2.6.2, is it possible to cherry pick to 2.6.3 as well @codelipenghui ?
----------------------------------------------------------------
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 #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#discussion_r537333916
##########
File path: .gitignore
##########
@@ -30,6 +30,9 @@ pulsar-functions/worker/src/test/resources/
*.iml
*.iws
+# NetBeans
+nb-configuration.xml
+
Review comment:
Seems not related to this PR?
----------------------------------------------------------------
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 #8725: Issue 8677: Cannot get lastMessageId for an empty topic due to message retention
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #8725:
URL: https://github.com/apache/pulsar/pull/8725#discussion_r532327986
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -2089,6 +2089,10 @@ void internalTrimConsumedLedgers(CompletableFuture<?> promise) {
log.debug("[{}] Ledger {} skipped for deletion as it is currently being written to", name,
ls.getLedgerId());
break;
+ } else if (currentLastConfirmedEntry != null && ls.getLedgerId() == currentLastConfirmedEntry.getLedgerId()) {
Review comment:
I am not sure this is the right fix. It changes the trim behavior which causes a rolled ledger to never be deleted until a new message is produced. I think a correct fix is to fix getLastMessageId behavior to correctly handle an empty ledger case.
----------------------------------------------------------------
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