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/07/05 02:52:26 UTC
[GitHub] [pulsar] codelipenghui opened a new pull request, #16389: [feature][broker] Support create ledger cursor lazily
codelipenghui opened a new pull request, #16389:
URL: https://github.com/apache/pulsar/pull/16389
### Motivation
Provide a way to create the ledger cursor lazily. It can reduce the subscription creation time-consuming.
In the case of millions of topics, consumers can complete subscriptions more quickly.
After enabling this feature, the cursor ledger will create will be created during the message acknowledgment.
If there are no message acknowledgments happened on a subscription, the cursor ledger will not be created.
### Modification
Added new configuration to enable the cursor ledger lazy creation
```
# Whether to create the cursor ledger lazily when recovering a managed cursor backing a durable subscription.
# It can reduce the subscription creation time-consuming. In the case of millions of topics, consumers can complete
# subscriptions more quickly.
#
# After enabling this option, the cursor ledger will create will be created during the message acknowledgment.
# If there are no message acknowledgments happened on a subscription, the cursor ledger will not be created.
# Default is false.
managedLedgerLazyCursorLedgerCreationEnabled=false
```
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [x] `doc-required`
(Your PR needs to update docs and you will update later)
- [ ] `doc-not-needed`
(Please explain why)
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
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 #16389: [improve][broker] Create the cursor ledger lazily to improve the subscribe performance
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16389:
URL: https://github.com/apache/pulsar/pull/16389#discussion_r920194365
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -647,18 +647,7 @@ void initialize(PositionImpl position, Map<String, Long> properties, Map<String,
ledger.getName(), name, messagesConsumedCounter, markDeletePosition, readPosition);
}
- createNewMetadataLedger(new VoidCallback() {
- @Override
- public void operationComplete() {
- STATE_UPDATER.set(ManagedCursorImpl.this, State.Open);
- callback.operationComplete();
- }
-
- @Override
- public void operationFailed(ManagedLedgerException exception) {
- callback.operationFailed(exception);
- }
- });
+ callback.operationComplete();
Review Comment:
Yes, correct. I have changed to persistPositionMetaStore().
--
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] merlimat merged pull request #16389: [improve][broker] Create the cursor ledger lazily to improve the subscribe performance
Posted by GitBox <gi...@apache.org>.
merlimat merged PR #16389:
URL: https://github.com/apache/pulsar/pull/16389
--
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 #16389: [improve][broker] Create the cursor ledger lazily to improve the subscribe performance
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16389:
URL: https://github.com/apache/pulsar/pull/16389#discussion_r920199611
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -3533,10 +3542,11 @@ public void testFlushCursorAfterError() throws Exception {
positions.add(ledger1.addEntry(new byte[1024]));
}
+ c1.markDelete(positions.get(0));
+ Thread.sleep(3000);
Review Comment:
This one is interesting, adding sleep here will work. Without the sleep, looks like the
```
bkc.failNow(BKException.Code.NotEnoughBookiesException);
metadataStore.setAlwaysFail(new MetadataStoreException.BadVersionException(""));
```
will not take effect. The mark delete operation will not get exception.
--
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] merlimat commented on a diff in pull request #16389: [improve][broker] Create the cursor ledger lazily to improve the subscribe performance
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #16389:
URL: https://github.com/apache/pulsar/pull/16389#discussion_r913387344
##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -3750,5 +3750,25 @@ public void readEntriesFailed(ManagedLedgerException exception, Object ctx) {
Awaitility.await().untilAsserted(() -> assertTrue(flag.get()));
}
+ @Test
+ public void testLazyCursorLedgerCreation() throws ManagedLedgerException, InterruptedException {
+ ManagedLedgerConfig managedLedgerConfig = new ManagedLedgerConfig();
+ ManagedLedgerImpl ledger = (ManagedLedgerImpl) factory
+ .open("testLazyCursorLedgerCreation", managedLedgerConfig);
+ ManagedCursorImpl cursor = (ManagedCursorImpl) ledger.openCursor("test");
+ assertEquals(cursor.getState(), "NoLedger");
Review Comment:
We should also assert the mark-delete position of the cursor before any ack is sent.
And we could close the cursor (either gracefully or not) and recover it to verify the mark-delete is correct.
To simulate the "ungraceful" close, we can use a different ManagedLedgerFactory to fence and recover the managed ledger.
--
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] merlimat commented on a diff in pull request #16389: [improve][broker] Create the cursor ledger lazily to improve the subscribe performance
Posted by GitBox <gi...@apache.org>.
merlimat commented on code in PR #16389:
URL: https://github.com/apache/pulsar/pull/16389#discussion_r914262023
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java:
##########
@@ -647,18 +647,7 @@ void initialize(PositionImpl position, Map<String, Long> properties, Map<String,
ledger.getName(), name, messagesConsumedCounter, markDeletePosition, readPosition);
}
- createNewMetadataLedger(new VoidCallback() {
- @Override
- public void operationComplete() {
- STATE_UPDATER.set(ManagedCursorImpl.this, State.Open);
- callback.operationComplete();
- }
-
- @Override
- public void operationFailed(ManagedLedgerException exception) {
- callback.operationFailed(exception);
- }
- });
+ callback.operationComplete();
Review Comment:
@codelipenghui There is a problem here.
The cursor z-node is not yet created at this point, so we are leaving the cursor as "ephemeral". It also means that the updates on the cursor z-node are then also failing because of the z-node version mismatch with the expectation.
In the current code, the cursor z-node is only created after:
1. Cursor ledger is created
2. We have written current mark-delete position to that ledger
3. Now we are going to create the cursor z-node (referencing the ledger id)
Instead of calling `operationComplete()` here we need to call `persistPositionMetaStore()` which will create the z-node and include the mark-delete position in the protobuf.
eg: sample test:
```java
@Test
public void testLazyCursorLedgerCreation() throws Exception {
ManagedLedgerConfig managedLedgerConfig = new ManagedLedgerConfig();
ManagedLedgerImpl ledger = (ManagedLedgerImpl) factory
.open("testLazyCursorLedgerCreation", managedLedgerConfig);
Position p1 = ledger.addEntry("test".getBytes());
ManagedCursorImpl cursor = (ManagedCursorImpl) ledger.openCursor("test");
assertEquals(cursor.getMarkDeletedPosition(), p1);
ManagedLedgerFactory factory2 = new ManagedLedgerFactoryImpl(metadataStore, bkc);
ledger = (ManagedLedgerImpl) factory2.open("testLazyCursorLedgerCreation", managedLedgerConfig);
assertNotNull(ledger.getCursors().get("test"));
ManagedCursorImpl cursor1 = (ManagedCursorImpl) ledger.openCursor("test");
assertEquals(cursor1.getMarkDeletedPosition(), p1);
factory2.shutdown();
}
```
--
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