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 2022/10/10 03:23:55 UTC
[GitHub] [pulsar] Pomelongan opened a new pull request, #17816: [fix][broker] add return for PersistentMessageExpiryMonitor#findEntryFailed
Pomelongan opened a new pull request, #17816:
URL: https://github.com/apache/pulsar/pull/17816
### Motivation
`org.apache.pulsar.broker.service.persistent.PersistentMessageExpiryMonitor#findEntryFailed` lacks a return, resulting in repeated execution of some codes.
### Modifications
add return for `org.apache.pulsar.broker.service.persistent.PersistentMessageExpiryMonitor#findEntryFailed`
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.
### Does this pull request potentially affect one of the following parts:
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] Anything that affects deployment
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
### Matching PR in forked repository
PR in forked repository: https://github.com/Pomelongan/pulsar/pull/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #17816: [improve][broker]add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #17816:
URL: https://github.com/apache/pulsar/pull/17816#discussion_r982980348
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java:
##########
@@ -192,6 +192,7 @@ public void findEntryFailed(ManagedLedgerException exception, Optional<Position>
log.warn("[{}][{}] read failed from ledger at position:{} : {}", topicName, subName, failedReadPosition,
exception.getMessage());
findEntryComplete(failedReadPosition.get(), ctx);
Review Comment:
@AnonHxy If we have completed the find entry task either succeeded or failed, we change `expirationCheckInProgress` to false, no? Otherwise, it looks like we will set `expirationCheckInProgress=true` before starting to find the entry, but the findEntryComplete and findEntryFailed can't ensure the `expirationCheckInProgress` change to false finally, it sounds confused.
--
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] codelipenghui closed pull request #17816: [fix][broker] add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
codelipenghui closed pull request #17816: [fix][broker] add return for PersistentMessageExpiryMonitor#findEntryFailed
URL: https://github.com/apache/pulsar/pull/17816
--
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] Jason918 commented on a diff in pull request #17816: [improve][broker]add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
Jason918 commented on code in PR #17816:
URL: https://github.com/apache/pulsar/pull/17816#discussion_r983035229
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java:
##########
@@ -192,6 +192,7 @@ public void findEntryFailed(ManagedLedgerException exception, Optional<Position>
log.warn("[{}][{}] read failed from ledger at position:{} : {}", topicName, subName, failedReadPosition,
exception.getMessage());
findEntryComplete(failedReadPosition.get(), ctx);
Review Comment:
@codelipenghui `expirationCheckInProgress` will be set to false in this `findEntryComplete(failedReadPosition.get(), ctx);`.
--
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] codelipenghui commented on a diff in pull request #17816: [improve][broker]add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #17816:
URL: https://github.com/apache/pulsar/pull/17816#discussion_r978439286
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java:
##########
@@ -192,6 +192,7 @@ public void findEntryFailed(ManagedLedgerException exception, Optional<Position>
log.warn("[{}][{}] read failed from ledger at position:{} : {}", topicName, subName, failedReadPosition,
exception.getMessage());
findEntryComplete(failedReadPosition.get(), ctx);
+ return;
Review Comment:
If we return here, how the `expirationCheckInProgress` will change to FALSE?
If the `expirationCheckInProgress` keep TRUE here. The subsequent message expiration task will always be skipped https://github.com/apache/pulsar/pull/17816/files#diff-813cddfda73ce8df0722a7c83ac1cb8d6d3db62ea13b44b5dc1bc438010fdd40R72
--
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] Jason918 commented on pull request #17816: [improve][broker]add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
Jason918 commented on PR #17816:
URL: https://github.com/apache/pulsar/pull/17816#issuecomment-1261719099
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Pomelongan commented on a diff in pull request #17816: [improve][broker]add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on code in PR #17816:
URL: https://github.com/apache/pulsar/pull/17816#discussion_r979527238
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java:
##########
@@ -192,6 +192,7 @@ public void findEntryFailed(ManagedLedgerException exception, Optional<Position>
log.warn("[{}][{}] read failed from ledger at position:{} : {}", topicName, subName, failedReadPosition,
exception.getMessage());
findEntryComplete(failedReadPosition.get(), ctx);
+ return;
Review Comment:
Thanks for reviewing～
The `findEntryComplete` method will change the `expirationCheckInProgress` to FALSE. see line182
https://github.com/apache/pulsar/blob/49eaa548b97eca06a6aff5d79391d49f085adb9e/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java#L168-L182
If return is not added, the `updateRates` method will execute repeatedly. see 👆line180 ,and see 👇line 1846 1856 1902 1911 1921
https://github.com/apache/pulsar/blob/49eaa548b97eca06a6aff5d79391d49f085adb9e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1840-L1925
and the implementation of `MarkDeleteCallback` is as follows，see line 150 163
https://github.com/apache/pulsar/blob/49eaa548b97eca06a6aff5d79391d49f085adb9e/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java#L144-L165
--
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] Pomelongan commented on a diff in pull request #17816: [improve][broker]add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on code in PR #17816:
URL: https://github.com/apache/pulsar/pull/17816#discussion_r979527238
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java:
##########
@@ -192,6 +192,7 @@ public void findEntryFailed(ManagedLedgerException exception, Optional<Position>
log.warn("[{}][{}] read failed from ledger at position:{} : {}", topicName, subName, failedReadPosition,
exception.getMessage());
findEntryComplete(failedReadPosition.get(), ctx);
+ return;
Review Comment:
Thanks for reviewing. The findEntryComplete method will change the `expirationCheckInProgress` to FALSE. If return is not added, the updateRates method will execute repeatedly
--
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] codelipenghui commented on a diff in pull request #17816: [improve][broker]add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #17816:
URL: https://github.com/apache/pulsar/pull/17816#discussion_r981153792
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java:
##########
@@ -192,6 +192,7 @@ public void findEntryFailed(ManagedLedgerException exception, Optional<Position>
log.warn("[{}][{}] read failed from ledger at position:{} : {}", topicName, subName, failedReadPosition,
exception.getMessage());
findEntryComplete(failedReadPosition.get(), ctx);
+ return;
Review Comment:
> The findEntryComplete method will change the expirationCheckInProgress to FALSE. see line182
Please see line 107. If we return here, the expirationCheckInProgress=true, we will not have a chance to call `findEntryComplete` method
--
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] AnonHxy commented on pull request #17816: [fix][broker] add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on PR #17816:
URL: https://github.com/apache/pulsar/pull/17816#issuecomment-1272939212
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Pomelongan commented on a diff in pull request #17816: [improve][broker]add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
Pomelongan commented on code in PR #17816:
URL: https://github.com/apache/pulsar/pull/17816#discussion_r979527238
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java:
##########
@@ -192,6 +192,7 @@ public void findEntryFailed(ManagedLedgerException exception, Optional<Position>
log.warn("[{}][{}] read failed from ledger at position:{} : {}", topicName, subName, failedReadPosition,
exception.getMessage());
findEntryComplete(failedReadPosition.get(), ctx);
+ return;
Review Comment:
@codelipenghui
Thanks for reviewing～
The `findEntryComplete` method will change the `expirationCheckInProgress` to FALSE. see line182
https://github.com/apache/pulsar/blob/49eaa548b97eca06a6aff5d79391d49f085adb9e/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java#L168-L182
If return is not added, the `updateRates` method will execute repeatedly. see 👆line180 ,and see 👇line 1846 1856 1902 1911 1921
https://github.com/apache/pulsar/blob/49eaa548b97eca06a6aff5d79391d49f085adb9e/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java#L1840-L1925
and the implementation of `MarkDeleteCallback` is as follows，see line 150 163
https://github.com/apache/pulsar/blob/49eaa548b97eca06a6aff5d79391d49f085adb9e/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java#L144-L165
--
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] AnonHxy commented on a diff in pull request #17816: [improve][broker]add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
AnonHxy commented on code in PR #17816:
URL: https://github.com/apache/pulsar/pull/17816#discussion_r982580764
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentMessageExpiryMonitor.java:
##########
@@ -192,6 +192,7 @@ public void findEntryFailed(ManagedLedgerException exception, Optional<Position>
log.warn("[{}][{}] read failed from ledger at position:{} : {}", topicName, subName, failedReadPosition,
exception.getMessage());
findEntryComplete(failedReadPosition.get(), ctx);
Review Comment:
It seems that here we just want to handle `findEntryFailed` like `findEntryComplete` if `autoSkipNonRecoverableData=true`. And `findEntryComplete` will change `expirationCheckInProgress` to false. So this change looks reasonable I think @Pomelongan @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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #17816: [fix][broker] add return for PersistentMessageExpiryMonitor#findEntryFailed
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #17816:
URL: https://github.com/apache/pulsar/pull/17816
--
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