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 2021/06/18 21:00:30 UTC
[GitHub] [pulsar] congbobo184 opened a new pull request #10966: [Broker] Fix get wrong backlog number.
congbobo184 opened a new pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966
## Motivation
Now when `managedLedger` create a new `ledger` complete. if `markDelete` is the `previousLedger` LAC it will delete the previousLedger from `managedLedger` . when get backlog we will use range.close to get `getNumberOfEntries` -1, if previousLedger not exist will get the wrong number.
## implement
use range.openClose() to `getBacklog`.
### Verifying this change
Add the tests for it
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] congbobo184 commented on a change in pull request #10966: [Broker] Fix get wrong backlog number.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966#discussion_r654740381
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java
##########
@@ -3526,5 +3528,26 @@ public void testCursorCheckReadPositionChanged() throws Exception {
ledger.close();
}
+
+ @Test // (timeOut = 20000)
+ public void testCursorGetBacklog() throws Exception {
+ ManagedLedgerConfig managedLedgerConfig = new ManagedLedgerConfig();
+ managedLedgerConfig.setMaxEntriesPerLedger(1);
+ managedLedgerConfig.setMinimumRolloverTime(0, TimeUnit.MILLISECONDS);
+ ManagedLedgerImpl ledger = (ManagedLedgerImpl) factory.open("get-backlog", managedLedgerConfig);
+ ManagedCursor managedCursor = ledger.openCursor("test");
+
+ Position position = ledger.addEntry("test".getBytes(Encoding));
+ ledger.addEntry("test".getBytes(Encoding));
+
+ Assert.assertEquals(managedCursor.getNumberOfEntriesInBacklog(true), 2);
+ Field field = ManagedLedgerImpl.class.getDeclaredField("ledgers");
+ field.setAccessible(true);
+
+ ((ConcurrentSkipListMap<Long, MLDataFormats.ManagedLedgerInfo.LedgerInfo>) field.get(ledger)).remove(position.getLedgerId());
Review comment:
test when the ledger have been deleted and the markDeletePosition is still the deleted ledger we can get the correct backlog.
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java
##########
@@ -3526,5 +3528,26 @@ public void testCursorCheckReadPositionChanged() throws Exception {
ledger.close();
}
+
+ @Test // (timeOut = 20000)
+ public void testCursorGetBacklog() throws Exception {
+ ManagedLedgerConfig managedLedgerConfig = new ManagedLedgerConfig();
+ managedLedgerConfig.setMaxEntriesPerLedger(1);
Review comment:
good idea.
--
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 #10966: [Broker] Fix the backlog issue with --precise-backlog=true
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966
--
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] 315157973 commented on a change in pull request #10966: [Broker] Fix get wrong backlog number.
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966#discussion_r654740921
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
##########
@@ -883,13 +883,13 @@ public long getNumberOfEntriesInBacklog(boolean isPrecise) {
messagesConsumedCounter, markDeletePosition, readPosition);
}
if (isPrecise) {
- return getNumberOfEntries(Range.closed(markDeletePosition, ledger.getLastPosition())) - 1;
+ return getNumberOfEntries(Range.openClosed(markDeletePosition, ledger.getLastPosition()));
}
long backlog = ManagedLedgerImpl.ENTRIES_ADDED_COUNTER_UPDATER.get(ledger) - messagesConsumedCounter;
if (backlog < 0) {
// In some case the counters get incorrect values, fall back to the precise backlog count
- backlog = getNumberOfEntries(Range.closed(markDeletePosition, ledger.getLastPosition())) - 1;
+ backlog = getNumberOfEntries(Range.openClosed(markDeletePosition, ledger.getLastPosition()));
Review comment:
Why don't we handle this situation inside the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] gaoran10 commented on pull request #10966: [Broker] Fix get wrong backlog number.
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966#issuecomment-863971221
I looked at the method `getNumberOfEntries` of the class `ManagedLedgerImpl`, if the `fromPosition` or `toPosition` is invalid, such as `3:-1`, the compute result will be wrong. If use the `openClosed` range will work around the problem. Maybe we could refine this method in next 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] gaoran10 commented on a change in pull request #10966: [Broker] Fix get wrong backlog number.
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966#discussion_r654771069
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
##########
@@ -883,13 +883,13 @@ public long getNumberOfEntriesInBacklog(boolean isPrecise) {
messagesConsumedCounter, markDeletePosition, readPosition);
}
if (isPrecise) {
- return getNumberOfEntries(Range.closed(markDeletePosition, ledger.getLastPosition())) - 1;
+ return getNumberOfEntries(Range.openClosed(markDeletePosition, ledger.getLastPosition()));
}
long backlog = ManagedLedgerImpl.ENTRIES_ADDED_COUNTER_UPDATER.get(ledger) - messagesConsumedCounter;
if (backlog < 0) {
// In some case the counters get incorrect values, fall back to the precise backlog count
- backlog = getNumberOfEntries(Range.closed(markDeletePosition, ledger.getLastPosition())) - 1;
+ backlog = getNumberOfEntries(Range.openClosed(markDeletePosition, ledger.getLastPosition()));
Review comment:
The range `openClosed` may be the appropriate select due to that the backlog metric doesn't include the `markDeletePosition` position.
--
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 #10966: [Broker] Fix get wrong backlog number.
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966#discussion_r654344017
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java
##########
@@ -3526,5 +3528,26 @@ public void testCursorCheckReadPositionChanged() throws Exception {
ledger.close();
}
+
+ @Test // (timeOut = 20000)
Review comment:
The param `timeOut` is need or not?
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java
##########
@@ -3526,5 +3528,26 @@ public void testCursorCheckReadPositionChanged() throws Exception {
ledger.close();
}
+
+ @Test // (timeOut = 20000)
+ public void testCursorGetBacklog() throws Exception {
+ ManagedLedgerConfig managedLedgerConfig = new ManagedLedgerConfig();
+ managedLedgerConfig.setMaxEntriesPerLedger(1);
Review comment:
Do we need test the scenario that one ledger maintain multiple entries?
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java
##########
@@ -3526,5 +3528,26 @@ public void testCursorCheckReadPositionChanged() throws Exception {
ledger.close();
}
+
+ @Test // (timeOut = 20000)
+ public void testCursorGetBacklog() throws Exception {
+ ManagedLedgerConfig managedLedgerConfig = new ManagedLedgerConfig();
+ managedLedgerConfig.setMaxEntriesPerLedger(1);
+ managedLedgerConfig.setMinimumRolloverTime(0, TimeUnit.MILLISECONDS);
+ ManagedLedgerImpl ledger = (ManagedLedgerImpl) factory.open("get-backlog", managedLedgerConfig);
+ ManagedCursor managedCursor = ledger.openCursor("test");
+
+ Position position = ledger.addEntry("test".getBytes(Encoding));
+ ledger.addEntry("test".getBytes(Encoding));
+
+ Assert.assertEquals(managedCursor.getNumberOfEntriesInBacklog(true), 2);
+ Field field = ManagedLedgerImpl.class.getDeclaredField("ledgers");
+ field.setAccessible(true);
+
+ ((ConcurrentSkipListMap<Long, MLDataFormats.ManagedLedgerInfo.LedgerInfo>) field.get(ledger)).remove(position.getLedgerId());
Review comment:
Maybe it's better to change the `markDeletePosition` at the same time, due to the range start position is `markDeletePosition`.
--
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 #10966: [Broker] Fix the backlog issue with --precise-backlog=true
Posted by GitBox <gi...@apache.org>.
codelipenghui merged pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966
--
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] congbobo184 commented on a change in pull request #10966: [Broker] Fix get wrong backlog number.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966#discussion_r654288949
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
##########
@@ -883,13 +883,13 @@ public long getNumberOfEntriesInBacklog(boolean isPrecise) {
messagesConsumedCounter, markDeletePosition, readPosition);
}
if (isPrecise) {
- return getNumberOfEntries(Range.closed(markDeletePosition, ledger.getLastPosition())) - 1;
+ return getNumberOfEntries(Range.openClosed(markDeletePosition, ledger.getLastPosition()));
Review comment:
when `precise = false` it will not process this logic
--
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] congbobo184 commented on a change in pull request #10966: [Broker] Fix get wrong backlog number.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on a change in pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966#discussion_r654740381
##########
File path: managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java
##########
@@ -3526,5 +3528,26 @@ public void testCursorCheckReadPositionChanged() throws Exception {
ledger.close();
}
+
+ @Test // (timeOut = 20000)
+ public void testCursorGetBacklog() throws Exception {
+ ManagedLedgerConfig managedLedgerConfig = new ManagedLedgerConfig();
+ managedLedgerConfig.setMaxEntriesPerLedger(1);
+ managedLedgerConfig.setMinimumRolloverTime(0, TimeUnit.MILLISECONDS);
+ ManagedLedgerImpl ledger = (ManagedLedgerImpl) factory.open("get-backlog", managedLedgerConfig);
+ ManagedCursor managedCursor = ledger.openCursor("test");
+
+ Position position = ledger.addEntry("test".getBytes(Encoding));
+ ledger.addEntry("test".getBytes(Encoding));
+
+ Assert.assertEquals(managedCursor.getNumberOfEntriesInBacklog(true), 2);
+ Field field = ManagedLedgerImpl.class.getDeclaredField("ledgers");
+ field.setAccessible(true);
+
+ ((ConcurrentSkipListMap<Long, MLDataFormats.ManagedLedgerInfo.LedgerInfo>) field.get(ledger)).remove(position.getLedgerId());
Review comment:
this test is for when the ledger have been deleted and the markDeletePosition is still the deleted ledger we can get the correct backlog.
--
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] congbobo184 edited a comment on pull request #10966: [Broker] Fix get wrong backlog number.
Posted by GitBox <gi...@apache.org>.
congbobo184 edited a comment on pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966#issuecomment-864348105
> I looked at the method `getNumberOfEntries` of the class `ManagedLedgerImpl`, if the `fromPosition` or `toPosition` is invalid, such as `3:-1`, the compute result will be wrong. If use the `openClosed` range will work around the problem. Maybe we could refine this method in next PR.
yes, now after use `getNumberOfEntries` will `-1` so we should find all the place where use `getNumberOfEntries` change it don't `-1` in the next 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] congbobo184 commented on pull request #10966: [Broker] Fix get wrong backlog number.
Posted by GitBox <gi...@apache.org>.
congbobo184 commented on pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966#issuecomment-864348105
> I looked at the method `getNumberOfEntries` of the class `ManagedLedgerImpl`, if the `fromPosition` or `toPosition` is invalid, such as `3:-1`, the compute result will be wrong. If use the `openClosed` range will work around the problem. Maybe we could refine this method in next PR.
yes, now after use `getNumberOfEntries` will `-1` so we should find all the place where use `getNumberOfEntries` and don't `-1` in the next 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] eolivelli commented on a change in pull request #10966: [Broker] Fix get wrong backlog number.
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #10966:
URL: https://github.com/apache/pulsar/pull/10966#discussion_r654286965
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
##########
@@ -883,13 +883,13 @@ public long getNumberOfEntriesInBacklog(boolean isPrecise) {
messagesConsumedCounter, markDeletePosition, readPosition);
}
if (isPrecise) {
- return getNumberOfEntries(Range.closed(markDeletePosition, ledger.getLastPosition())) - 1;
+ return getNumberOfEntries(Range.openClosed(markDeletePosition, ledger.getLastPosition()));
Review comment:
in your test you are exercising only this case with "precise = true"
can you add a test with precise = false ?
--
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