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/12/12 14:47:02 UTC
[GitHub] [pulsar] gaoran10 opened a new pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
gaoran10 opened a new pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938
### Motivation
Currently, the current ledger maybe adds to the offload deque.
### Modifications
Exclude current ledger from offload deque.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### 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: (no)
- 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] eolivelli commented on a change in pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#discussion_r542170088
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -1411,9 +1411,21 @@ synchronized void ledgerClosed(final LedgerHandle lh) {
log.debug("[{}] Ledger has been closed id={} entries={}", name, lh.getId(), entriesInLedger);
}
if (entriesInLedger > 0) {
- LedgerInfo info = LedgerInfo.newBuilder().setLedgerId(lh.getId()).setEntries(entriesInLedger)
- .setSize(lh.getLength()).setTimestamp(clock.millis()).build();
- ledgers.put(lh.getId(), info);
+ synchronized (ledgers) {
Review comment:
We are not using synchronized on ledgers variable in other points in this class.
It looks to me as an inconsistent synchronisation.
----------------------------------------------------------------
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] gaoran10 commented on pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#issuecomment-748010783
/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] gaoran10 commented on pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#issuecomment-747874138
/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] wolfstudy commented on pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#issuecomment-748963262
@gaoran10 Please check the action CI, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on a change in pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#discussion_r545578288
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -1411,9 +1411,21 @@ synchronized void ledgerClosed(final LedgerHandle lh) {
log.debug("[{}] Ledger has been closed id={} entries={}", name, lh.getId(), entriesInLedger);
}
if (entriesInLedger > 0) {
- LedgerInfo info = LedgerInfo.newBuilder().setLedgerId(lh.getId()).setEntries(entriesInLedger)
- .setSize(lh.getLength()).setTimestamp(clock.millis()).build();
- ledgers.put(lh.getId(), info);
+ synchronized (ledgers) {
Review comment:
Thanks, eolivelli, I think you are right.
I checked the code and found out all situations that change the `ledgers` in the `ManagedLedgerImpl`, for a ledger its `LedgerInfo` update operations don't exist competition.
1. When the ManagedLedgerImpl initialized, recover metadata from the metadata store.
2. When creating a new ledger.
3. When the ledger closed.
4. When the offloaded ledger data should be deleted from bk.
5. When preparing for offload.
6. When offloading complete.
I think the `LedgerInfo` should be initialized when the ledger created, after this, the `LedgerInfo` should be updated not be created.
PTAL
----------------------------------------------------------------
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] gaoran10 commented on pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#issuecomment-749499907
The current ledge would not be added to the offload queue, I 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#issuecomment-747875182
@sijie Ok, I'll try to add a test, but it's not easy to cover the problem.
----------------------------------------------------------------
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] gaoran10 commented on pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#issuecomment-748705625
/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] gaoran10 commented on a change in pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#discussion_r545578288
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -1411,9 +1411,21 @@ synchronized void ledgerClosed(final LedgerHandle lh) {
log.debug("[{}] Ledger has been closed id={} entries={}", name, lh.getId(), entriesInLedger);
}
if (entriesInLedger > 0) {
- LedgerInfo info = LedgerInfo.newBuilder().setLedgerId(lh.getId()).setEntries(entriesInLedger)
- .setSize(lh.getLength()).setTimestamp(clock.millis()).build();
- ledgers.put(lh.getId(), info);
+ synchronized (ledgers) {
Review comment:
Thanks, eolivelli, I think you are right.
I checked the code and found out all situations that change the `ledgers` in the `ManagedLedgerImpl`, for a ledger its `LedgerInfo` update operations don't exist competition.
1. When the ManagedLedgerImpl initialized, recover metadata from the metadata store.
2. When creating a new ledger.
3. When the ledger closed.
4. When the offloaded ledger data should be deleted from bk.
5. When preparing for offload.
6. When offloading complete.
I think the `LedgerInfo` should be initialized when the ledger created, after this, the `LedgerInfo` should be updated, not be created.
PTAL
----------------------------------------------------------------
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] gaoran10 commented on a change in pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#discussion_r545578288
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
##########
@@ -1411,9 +1411,21 @@ synchronized void ledgerClosed(final LedgerHandle lh) {
log.debug("[{}] Ledger has been closed id={} entries={}", name, lh.getId(), entriesInLedger);
}
if (entriesInLedger > 0) {
- LedgerInfo info = LedgerInfo.newBuilder().setLedgerId(lh.getId()).setEntries(entriesInLedger)
- .setSize(lh.getLength()).setTimestamp(clock.millis()).build();
- ledgers.put(lh.getId(), info);
+ synchronized (ledgers) {
Review comment:
Thanks, eolivelli, I think you are right.
I checked the code and found out all situations that change the `ledgers` in the `ManagedLedgerImpl`, for a ledger its `LedgerInfo` update operations don't exist competition.
1. When the ManagedLedgerImpl initialized, recover metadata from the metadata store.
2. When creating a new ledger.
3. When the ledger closed.
4. When the offloaded ledger data should be deleted from bk.
5. When preparing for offload.
6. When offloading complete.
I think the `LedgerInfo` should be initialized when the ledger created, after this, the `LedgerInfo` should be updated, not be created repeatedly.
PTAL
----------------------------------------------------------------
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] wolfstudy commented on pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#issuecomment-746641542
@gaoran10 can you check @eolivelli comments? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938#issuecomment-748049293
/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] gaoran10 closed pull request #8938: [Tiered Storage] Exclude current ledger from offload deque
Posted by GitBox <gi...@apache.org>.
gaoran10 closed pull request #8938:
URL: https://github.com/apache/pulsar/pull/8938
----------------------------------------------------------------
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