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 2023/01/03 15:10:15 UTC

[GitHub] [pulsar] coderzc opened a new pull request, #19124: [fix][broker] fix cursor skip read

coderzc opened a new pull request, #19124:
URL: https://github.com/apache/pulsar/pull/19124

   PIP: #16763
   
   <!-- Details of when a PIP is required and how the PIP process work, please see: https://github.com/apache/pulsar/blob/master/wiki/proposals/PIP.md -->
   
   ### Motivation
   
   The `skipCondition` parameter returns need to skip entry when reading entry, but the logic now is reversed.
   
   ### Modifications
   
   Fix skip logic and add test to cover it.
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI checks.
   
   *testReadEntriesWithFilterOut*
   
   ### Documentation
   
   <!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
   
   - [ ] `doc` <!-- Your PR contains doc changes. Please attach the local preview screenshots (run `sh start.sh` at `pulsar/site2/website`) to your PR description, or else your PR might not get merged. -->
   - [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
   - [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
   - [ ] `doc-complete` <!-- Docs have been already added -->
   
   ### Matching PR in forked repository
   
   PR in forked repository: <!-- ENTER URL HERE -->
   
   <!--
   After opening this PR, the build in apache/pulsar will fail and instructions will
   be provided for opening a PR in the PR author's forked repository.
   
   apache/pulsar pull requests should be first tested in your own fork since the 
   apache/pulsar CI based on GitHub Actions has constrained resources and quota.
   GitHub Actions provides separate quota for pull requests that are executed in 
   a forked repository.
   
   The tests will be run in the forked repository until all PR review comments have
   been handled, the tests pass and the PR is approved by a reviewer.
   -->
   


-- 
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] mattisonchao commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061127164


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2060,7 +2060,7 @@ private void internalReadFromLedger(ReadHandle ledger, OpReadEntry opReadEntry)
             long lastValidEntry = -1L;
             long entryId = firstEntry;
             for (; entryId <= lastEntry; entryId++) {
-                if (opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
+                if (!opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {

Review Comment:
   I suggest Invert if condition to help simple understand 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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061110082


##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -4275,7 +4275,14 @@ public void testReadEntriesWithFilterOut() throws ManagedLedgerException, Interr
         cursor.asyncReadEntriesWithSkipOrWait(sendNumber, new ReadEntriesCallback() {
             @Override
             public void readEntriesComplete(List<Entry> entries, Object ctx) {
-                completableFuture.complete(entries.size());
+                try {
+                    entries.forEach(entry -> {
+                        assertNotEquals(entry.getEntryId() % 2, 0);

Review Comment:
   > And due to this one is executed by the SafeRun, so the test will not get failed event if the assert fails.
   
   I already use `completableFuture.completeExceptionally(e)` to receive exception, it should be can work.
   
   But `AssertionError` is not a subclass of `Exception`, so we should be caught `Throwable`.



-- 
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] coderzc commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061107452


##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -4275,7 +4275,14 @@ public void testReadEntriesWithFilterOut() throws ManagedLedgerException, Interr
         cursor.asyncReadEntriesWithSkipOrWait(sendNumber, new ReadEntriesCallback() {
             @Override
             public void readEntriesComplete(List<Entry> entries, Object ctx) {
-                completableFuture.complete(entries.size());
+                try {
+                    entries.forEach(entry -> {
+                        assertNotEquals(entry.getEntryId() % 2, 0);

Review Comment:
   > It should be `assertEquals(entry.getEntryId() % 2, 0);`
   
   It should be `assertSame(entry.getEntryId() % 2, 0);`



-- 
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 #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #19124:
URL: https://github.com/apache/pulsar/pull/19124


-- 
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] coderzc commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061380188


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2060,14 +2060,14 @@ private void internalReadFromLedger(ReadHandle ledger, OpReadEntry opReadEntry)
             long lastValidEntry = -1L;
             long entryId = firstEntry;
             for (; entryId <= lastEntry; entryId++) {
-                if (!opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
-                    if (firstValidEntry == -1L) {
-                        firstValidEntry = entryId;
-                    }
-                } else {
+                if (opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
                     if (firstValidEntry != -1L) {
                         break;
                     }
+                } else {
+                    if (firstValidEntry == -1L) {
+                        firstValidEntry = entryId;
+                    }
                 }
 
                 if (firstValidEntry != -1L) {

Review Comment:
   Yes, we can move it to the `else` branch



-- 
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] mattisonchao commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061358571


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2060,14 +2060,14 @@ private void internalReadFromLedger(ReadHandle ledger, OpReadEntry opReadEntry)
             long lastValidEntry = -1L;
             long entryId = firstEntry;
             for (; entryId <= lastEntry; entryId++) {
-                if (!opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
-                    if (firstValidEntry == -1L) {
-                        firstValidEntry = entryId;
-                    }
-                } else {
+                if (opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
                     if (firstValidEntry != -1L) {
                         break;
                     }
+                } else {
+                    if (firstValidEntry == -1L) {
+                        firstValidEntry = entryId;
+                    }
                 }
 
                 if (firstValidEntry != -1L) {

Review Comment:
   It looks like this logic just works for the `else` branch. Can we merge 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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061110082


##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -4275,7 +4275,14 @@ public void testReadEntriesWithFilterOut() throws ManagedLedgerException, Interr
         cursor.asyncReadEntriesWithSkipOrWait(sendNumber, new ReadEntriesCallback() {
             @Override
             public void readEntriesComplete(List<Entry> entries, Object ctx) {
-                completableFuture.complete(entries.size());
+                try {
+                    entries.forEach(entry -> {
+                        assertNotEquals(entry.getEntryId() % 2, 0);

Review Comment:
   > And due to this one is executed by the SafeRun, so the test will not get failed event if the assert fails.
   
   I already use `completableFuture.completeExceptionally(e)` to receive exception, it should be can work.
   
   But `AssertionError` is not a subclass of `Exception`, so we should be caught `Throwable`.
   
   <img width="1076" alt="image" src="https://user-images.githubusercontent.com/26179648/210480680-dd6983fe-1709-4fe6-82a1-c0d189058de2.png">
   
   



-- 
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 #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061102602


##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -4275,7 +4275,14 @@ public void testReadEntriesWithFilterOut() throws ManagedLedgerException, Interr
         cursor.asyncReadEntriesWithSkipOrWait(sendNumber, new ReadEntriesCallback() {
             @Override
             public void readEntriesComplete(List<Entry> entries, Object ctx) {
-                completableFuture.complete(entries.size());
+                try {
+                    entries.forEach(entry -> {
+                        assertNotEquals(entry.getEntryId() % 2, 0);

Review Comment:
   It should be `assertEquals(entry.getEntryId() % 2, 0);`
   
   And due to this one is executed by the SafeRun, so the test will not get failed event if the assert fails.



##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -4295,7 +4302,14 @@ public void readEntriesFailed(ManagedLedgerException exception, Object ctx) {
         cursor.asyncReadEntriesWithSkipOrWait(sendNumber, new ReadEntriesCallback() {
             @Override
             public void readEntriesComplete(List<Entry> entries, Object ctx) {
-                completableFuture2.complete(entries.size());
+                try {
+                    entries.forEach(entry -> {
+                        assertEquals(entry.getEntryId() % 2, 0);

Review Comment:
   Same as the above comment. You might need to consider using a different way to assert the result.



-- 
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] coderzc commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061110082


##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -4275,7 +4275,14 @@ public void testReadEntriesWithFilterOut() throws ManagedLedgerException, Interr
         cursor.asyncReadEntriesWithSkipOrWait(sendNumber, new ReadEntriesCallback() {
             @Override
             public void readEntriesComplete(List<Entry> entries, Object ctx) {
-                completableFuture.complete(entries.size());
+                try {
+                    entries.forEach(entry -> {
+                        assertNotEquals(entry.getEntryId() % 2, 0);

Review Comment:
   > And due to this one is executed by the SafeRun, so the test will not get failed event if the assert fails.
   
   I already use `completableFuture.completeExceptionally(e)` to receive exception, it should be can work.
   
   But `AssertionError` is not a subclass of `Exception`, so we should be caught `Throwable`.
   
   <img width="1118" alt="image" src="https://user-images.githubusercontent.com/26179648/210480197-57ac61e8-6c97-43c2-bae1-a8fb6de73deb.png">
   



-- 
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] mattisonchao commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061352981


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2060,14 +2060,14 @@ private void internalReadFromLedger(ReadHandle ledger, OpReadEntry opReadEntry)
             long lastValidEntry = -1L;
             long entryId = firstEntry;
             for (; entryId <= lastEntry; entryId++) {
-                if (!opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
-                    if (firstValidEntry == -1L) {
-                        firstValidEntry = entryId;
-                    }
-                } else {
+                if (opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
                     if (firstValidEntry != -1L) {
                         break;
                     }
+                } else {
+                    if (firstValidEntry == -1L) {
+                        firstValidEntry = entryId;
+                    }
                 }
 
                 if (firstValidEntry != -1L) {

Review Comment:
   I think this logic just works for the `else` branch. Can we merge 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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] codecov-commenter commented on pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#issuecomment-1369923908

   # [Codecov](https://codecov.io/gh/apache/pulsar/pull/19124?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#19124](https://codecov.io/gh/apache/pulsar/pull/19124?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f1a7bd7) into [master](https://codecov.io/gh/apache/pulsar/commit/9ec1d071c7188a2db694e9d7b359faaf33cb076e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9ec1d07) will **decrease** coverage by `10.03%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pulsar/pull/19124/graphs/tree.svg?width=650&height=150&src=pr&token=acYqCpsK9J&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pulsar/pull/19124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@              Coverage Diff              @@
   ##             master   #19124       +/-   ##
   =============================================
   - Coverage     47.43%   37.39%   -10.04%     
   + Complexity     9535     1998     -7537     
   =============================================
     Files           632      209      -423     
     Lines         59839    14452    -45387     
     Branches       6234     1577     -4657     
   =============================================
   - Hits          28384     5405    -22979     
   + Misses        28375     8455    -19920     
   + Partials       3080      592     -2488     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | unittests | `37.39% <ø> (-10.04%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/19124?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...g/apache/pulsar/client/impl/ConnectionHandler.java](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL0Nvbm5lY3Rpb25IYW5kbGVyLmphdmE=) | `50.00% <0.00%> (-5.32%)` | :arrow_down: |
   | [...oker/stats/prometheus/AggregatedConsumerStats.java](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zdGF0cy9wcm9tZXRoZXVzL0FnZ3JlZ2F0ZWRDb25zdW1lclN0YXRzLmphdmE=) | | |
   | [...er/service/persistent/GeoPersistentReplicator.java](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvR2VvUGVyc2lzdGVudFJlcGxpY2F0b3IuamF2YQ==) | | |
   | [.../apache/pulsar/broker/service/EntryBatchSizes.java](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0VudHJ5QmF0Y2hTaXplcy5qYXZh) | | |
   | [...rg/apache/pulsar/broker/web/PulsarWebResource.java](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci93ZWIvUHVsc2FyV2ViUmVzb3VyY2UuamF2YQ==) | | |
   | [...lsar/broker/service/PublishRateLimiterDisable.java](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1B1Ymxpc2hSYXRlTGltaXRlckRpc2FibGUuamF2YQ==) | | |
   | [...pulsar/broker/admin/v2/ExtNonPersistentTopics.java](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9FeHROb25QZXJzaXN0ZW50VG9waWNzLmphdmE=) | | |
   | [.../org/apache/pulsar/broker/service/PulsarStats.java](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1B1bHNhclN0YXRzLmphdmE=) | | |
   | [.../apache/pulsar/proxy/server/AdminProxyHandler.java](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLXByb3h5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9wdWxzYXIvcHJveHkvc2VydmVyL0FkbWluUHJveHlIYW5kbGVyLmphdmE=) | | |
   | [...g/apache/pulsar/broker/admin/impl/SourcesBase.java](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi9pbXBsL1NvdXJjZXNCYXNlLmphdmE=) | | |
   | ... and [419 more](https://codecov.io/gh/apache/pulsar/pull/19124/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   


-- 
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] coderzc closed pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
coderzc closed pull request #19124: [fix][broker][PIP-195] fix cursor skip read
URL: https://github.com/apache/pulsar/pull/19124


-- 
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] coderzc commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061110082


##########
managed-ledger/src/test/java/org/apache/bookkeeper/mledger/impl/ManagedCursorTest.java:
##########
@@ -4275,7 +4275,14 @@ public void testReadEntriesWithFilterOut() throws ManagedLedgerException, Interr
         cursor.asyncReadEntriesWithSkipOrWait(sendNumber, new ReadEntriesCallback() {
             @Override
             public void readEntriesComplete(List<Entry> entries, Object ctx) {
-                completableFuture.complete(entries.size());
+                try {
+                    entries.forEach(entry -> {
+                        assertNotEquals(entry.getEntryId() % 2, 0);

Review Comment:
   > And due to this one is executed by the SafeRun, so the test will not get failed event if the assert fails.
   
   I already use `completableFuture.completeExceptionally(e)` to receive exception, it should be can work.



-- 
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] coderzc commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061224384


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2060,7 +2060,7 @@ private void internalReadFromLedger(ReadHandle ledger, OpReadEntry opReadEntry)
             long lastValidEntry = -1L;
             long entryId = firstEntry;
             for (; entryId <= lastEntry; entryId++) {
-                if (opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
+                if (!opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {

Review Comment:
   Ok.



-- 
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] mattisonchao commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061355101


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2060,14 +2060,14 @@ private void internalReadFromLedger(ReadHandle ledger, OpReadEntry opReadEntry)
             long lastValidEntry = -1L;
             long entryId = firstEntry;
             for (; entryId <= lastEntry; entryId++) {
-                if (!opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
-                    if (firstValidEntry == -1L) {
-                        firstValidEntry = entryId;
-                    }
-                } else {
+                if (opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
                     if (firstValidEntry != -1L) {
                         break;
                     }
+                } else {
+                    if (firstValidEntry == -1L) {
+                        firstValidEntry = entryId;
+                    }
                 }
 
                 if (firstValidEntry != -1L) {

Review Comment:
   It looks like this logic just works for the `else` branch. Can we merge 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.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] coderzc commented on a diff in pull request #19124: [fix][broker][PIP-195] fix cursor skip read

Posted by GitBox <gi...@apache.org>.
coderzc commented on code in PR #19124:
URL: https://github.com/apache/pulsar/pull/19124#discussion_r1061380188


##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java:
##########
@@ -2060,14 +2060,14 @@ private void internalReadFromLedger(ReadHandle ledger, OpReadEntry opReadEntry)
             long lastValidEntry = -1L;
             long entryId = firstEntry;
             for (; entryId <= lastEntry; entryId++) {
-                if (!opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
-                    if (firstValidEntry == -1L) {
-                        firstValidEntry = entryId;
-                    }
-                } else {
+                if (opReadEntry.skipCondition.test(PositionImpl.get(ledger.getId(), entryId))) {
                     if (firstValidEntry != -1L) {
                         break;
                     }
+                } else {
+                    if (firstValidEntry == -1L) {
+                        firstValidEntry = entryId;
+                    }
                 }
 
                 if (firstValidEntry != -1L) {

Review Comment:
   Yes, we can move this logic to the `else` branch



-- 
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