[fix] [txn] Get previous position by managed ledger.

[thetumbled]

Motivation

Max read position is before the first ongoing txn's position. But the entry id of the first message may be 0, calculate the max read position by entry id - 1 is unsafe.

Modifications

Calculate the max read position by method managedLedger.getPreviousPosition.

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.

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 threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository: thetumbled#42

[congbobo184]

please add a test for this change

[dao-jun]

please add a test for this change

I think existing tests could cover the case.

[thetumbled]

PTAL, thanks. @dao-jun @congbobo184 @liangyepianzhou

[thetumbled]

/pulsarbot rerun-failure-checks

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (825e997) 73.57% compared to head (b9b8913) 73.54%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #22024      +/-   ##
============================================
- Coverage     73.57%   73.54%   -0.03%     
+ Complexity    32575    32049     -526     
============================================
  Files          1874     1874              
  Lines        139252   139252              
  Branches      15260    15260              
============================================
- Hits         102454   102413      -41     
- Misses        28880    28916      +36     
- Partials       7918     7923       +5     

Flag	Coverage Δ
inttests	24.63% <100.00%> (-0.15%)	⬇️
systests	24.26% <0.00%> (-0.09%)	⬇️
unittests	72.81% <100.00%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...ransaction/buffer/impl/TopicTransactionBuffer.java	87.74% <100.00%> (ø)

... and 71 files with indirect coverage changes

[liangyepianzhou]

Good catch. Please help supplement the testing.

[thetumbled]

Hi, I have add a test to produce a exception, and it is necessary to fix this problem.
PTAL, thanks. @dao-jun @congbobo184 @liangyepianzhou

[Technoboy-]

@dao-jun It's better to check which branches we need to release before merging.
@liangyepianzhou please help label the branches

[heesung-sn]

This fails the build in branch-3.0 now. Please help to fix this issue.

@Technoboy- @thetumbled

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project pulsar-broker: Compilation failure
[ERROR] /Users/hsohn/Workspace/pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:[286,33] cannot find symbol
[ERROR]   symbol:   class ManagedLedgerImpl
[ERROR]   location: class org.apache.pulsar.broker.transaction.buffer.impl.TopicTransactionBuffer

[lhotari]

This fails the build in branch-3.0 now. Please help to fix this issue.

@Technoboy- @thetumbled

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project pulsar-broker: Compilation failure
[ERROR] /Users/hsohn/Workspace/pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:[286,33] cannot find symbol
[ERROR]   symbol:   class ManagedLedgerImpl
[ERROR]   location: class org.apache.pulsar.broker.transaction.buffer.impl.TopicTransactionBuffer

@Technoboy- can this be reverted from branch-3.0 ?

[thetumbled]

This fails the build in branch-3.0 now. Please help to fix this issue.
@Technoboy- @thetumbled

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.11.0:compile (default-compile) on project pulsar-broker: Compilation failure
[ERROR] /Users/hsohn/Workspace/pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:[286,33] cannot find symbol
[ERROR]   symbol:   class ManagedLedgerImpl
[ERROR]   location: class org.apache.pulsar.broker.transaction.buffer.impl.TopicTransactionBuffer

@Technoboy- can this be reverted from branch-3.0 ?

It seems that the problem has been fixed by @Technoboy.

[lhotari]

It seems that the problem has been fixed by @Technoboy.

@thetumbled How was it fixed? we need the fix to branch-3.0 .

[thetumbled]

It seems that the problem has been fixed by @Technoboy.

@thetumbled How was it fixed? we need the fix to branch-3.0 .

commit: 5578873

[lhotari]

@thetumbled Thanks. There's a test failure in branch-3.0 too, to reproduce:

mvn -Pcore-modules,-main -T 1C clean -DskipTests -Dspotbugs.skip=true
mvn -DredirectTestOutputToFile=false -DtestRetryCount=0 test -pl pulsar-broker -Dtest=TopicTransactionBufferTest

[thetumbled]

@thetumbled Thanks. There's a test failure in branch-3.0 too, to reproduce:

mvn -Pcore-modules,-main -T 1C clean -DskipTests -Dspotbugs.skip=true
mvn -DredirectTestOutputToFile=false -DtestRetryCount=0 test -pl pulsar-broker -Dtest=TopicTransactionBufferTest

Let me take a look at this issue.

[Technoboy-]

TopicTransactionBufferTest.testGetLastMessageIdsWithOngoingTransactions

Revert this pr and the test still failed

[thetumbled]

@thetumbled Thanks. There's a test failure in branch-3.0 too, to reproduce:

mvn -Pcore-modules,-main -T 1C clean -DskipTests -Dspotbugs.skip=true
mvn -DredirectTestOutputToFile=false -DtestRetryCount=0 test -pl pulsar-broker -Dtest=TopicTransactionBufferTest

The problem is fixed by pr: #22130