You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "thetumbled (via GitHub)" <gi...@apache.org> on 2024/02/05 08:01:45 UTC
[PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
thetumbled opened a new pull request, #22024:
URL: https://github.com/apache/pulsar/pull/22024
### 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:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*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
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `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 -->
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1965943455
> @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.
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun merged PR #22024:
URL: https://github.com/apache/pulsar/pull/22024
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1953440822
PTAL, thanks. @dao-jun @congbobo184 @liangyepianzhou
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1965887609
> It seems that the problem has been fixed by @Technoboy.
@thetumbled How was it fixed? we need the fix to branch-3.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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1956034191
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/22024?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`825e997`)](https://app.codecov.io/gh/apache/pulsar/commit/825e997216dabe23a6dde0945ef769bbda0558e4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 73.57% compared to head [(`b9b8913`)](https://app.codecov.io/gh/apache/pulsar/pull/22024?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 73.54%.
> Report is 2 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/pulsar/pull/22024?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ 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](https://app.codecov.io/gh/apache/pulsar/pull/22024/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [inttests](https://app.codecov.io/gh/apache/pulsar/pull/22024/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.63% <100.00%> (-0.15%)` | :arrow_down: |
| [systests](https://app.codecov.io/gh/apache/pulsar/pull/22024/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.26% <0.00%> (-0.09%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pulsar/pull/22024/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.81% <100.00%> (-0.03%)` | :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=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files](https://app.codecov.io/gh/apache/pulsar/pull/22024?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ransaction/buffer/impl/TopicTransactionBuffer.java](https://app.codecov.io/gh/apache/pulsar/pull/22024?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci90cmFuc2FjdGlvbi9idWZmZXIvaW1wbC9Ub3BpY1RyYW5zYWN0aW9uQnVmZmVyLmphdmE=) | `87.74% <100.00%> (ø)` | |
... and [71 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/22024/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1965921600
@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
```
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1965650503
> > 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.
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#discussion_r1493924929
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -287,8 +287,8 @@ private void handleTransactionMessage(TxnID txnId, Position position) {
.checkAbortedTransaction(txnId)) {
ongoingTxns.put(txnId, (PositionImpl) position);
PositionImpl firstPosition = ongoingTxns.get(ongoingTxns.firstKey());
- //max read position is less than first ongoing transaction message position, so entryId -1
- maxReadPosition = PositionImpl.get(firstPosition.getLedgerId(), firstPosition.getEntryId() - 1);
+ // max read position is less than first ongoing transaction message position
+ maxReadPosition = getPreviousPosition(firstPosition);
Review Comment:
There are two places that we need to get the previous position, but they are not consistent. We have better make them consistent.
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1951377333
> please add a test for this change
I think existing tests could cover the case.
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1958886479
@dao-jun It's better to check which branches we need to release before merging.
@liangyepianzhou please help label the branches
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#discussion_r1479124309
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -287,8 +287,8 @@ private void handleTransactionMessage(TxnID txnId, Position position) {
.checkAbortedTransaction(txnId)) {
ongoingTxns.put(txnId, (PositionImpl) position);
PositionImpl firstPosition = ongoingTxns.get(ongoingTxns.firstKey());
- //max read position is less than first ongoing transaction message position, so entryId -1
- maxReadPosition = PositionImpl.get(firstPosition.getLedgerId(), firstPosition.getEntryId() - 1);
+ // max read position is less than first ongoing transaction message position, so entryId -1
Review Comment:
done.
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1965953836
```
TopicTransactionBufferTest.testGetLastMessageIdsWithOngoingTransactions
```
Revert this pr and the test still failed
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1966100585
> @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: https://github.com/apache/pulsar/pull/22130
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#discussion_r1493927571
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -442,14 +442,22 @@ private void takeSnapshotByTimeout() {
takeSnapshotIntervalTime, TimeUnit.MILLISECONDS);
}
+ private PositionImpl getPreviousPosition(PositionImpl position) {
+ return ((ManagedLedgerImpl) topic.getManagedLedger()).getPreviousPosition(position);
+ }
+
+ private PositionImpl getLastConfirmedEntry() {
Review Comment:
fixed.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -442,14 +442,22 @@ private void takeSnapshotByTimeout() {
takeSnapshotIntervalTime, TimeUnit.MILLISECONDS);
}
+ private PositionImpl getPreviousPosition(PositionImpl position) {
Review Comment:
fixed.
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on code in PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#discussion_r1496938782
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -287,8 +287,8 @@ private void handleTransactionMessage(TxnID txnId, Position position) {
.checkAbortedTransaction(txnId)) {
ongoingTxns.put(txnId, (PositionImpl) position);
PositionImpl firstPosition = ongoingTxns.get(ongoingTxns.firstKey());
- //max read position is less than first ongoing transaction message position, so entryId -1
- maxReadPosition = PositionImpl.get(firstPosition.getLedgerId(), firstPosition.getEntryId() - 1);
+ // max read position is less than first ongoing transaction message position
+ maxReadPosition = getPreviousPosition(firstPosition);
Review Comment:
it makes sense
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on code in PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#discussion_r1493800083
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -442,14 +442,22 @@ private void takeSnapshotByTimeout() {
takeSnapshotIntervalTime, TimeUnit.MILLISECONDS);
}
+ private PositionImpl getPreviousPosition(PositionImpl position) {
+ return ((ManagedLedgerImpl) topic.getManagedLedger()).getPreviousPosition(position);
+ }
+
+ private PositionImpl getLastConfirmedEntry() {
Review Comment:
seems we don't need the method.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -287,8 +287,8 @@ private void handleTransactionMessage(TxnID txnId, Position position) {
.checkAbortedTransaction(txnId)) {
ongoingTxns.put(txnId, (PositionImpl) position);
PositionImpl firstPosition = ongoingTxns.get(ongoingTxns.firstKey());
- //max read position is less than first ongoing transaction message position, so entryId -1
- maxReadPosition = PositionImpl.get(firstPosition.getLedgerId(), firstPosition.getEntryId() - 1);
+ // max read position is less than first ongoing transaction message position
+ maxReadPosition = getPreviousPosition(firstPosition);
Review Comment:
In fact, `entryId=-1` does not cause program exceptions, There are many uses of this in pulsar. But if modified here, it is also acceptable
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -442,14 +442,22 @@ private void takeSnapshotByTimeout() {
takeSnapshotIntervalTime, TimeUnit.MILLISECONDS);
}
+ private PositionImpl getPreviousPosition(PositionImpl position) {
Review Comment:
remove the method and use ` topic.getManagedLedger()).getPreviousPosition(position);` directly is enough.
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1965896997
> > It seems that the problem has been fixed by @Technoboy.
>
> @thetumbled How was it fixed? we need the fix to branch-3.0 .
commit: 55788734a53acd505d97e6f8b3b4bd567603d9ce
<img width="508" alt="image" src="https://github.com/apache/pulsar/assets/52550727/4f5ca094-1926-4844-b6eb-75b744a6646c">
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "heesung-sn (via GitHub)" <gi...@apache.org>.
heesung-sn commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1960602889
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
```
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "liangyepianzhou (via GitHub)" <gi...@apache.org>.
liangyepianzhou commented on code in PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#discussion_r1478077976
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -287,8 +287,8 @@ private void handleTransactionMessage(TxnID txnId, Position position) {
.checkAbortedTransaction(txnId)) {
ongoingTxns.put(txnId, (PositionImpl) position);
PositionImpl firstPosition = ongoingTxns.get(ongoingTxns.firstKey());
- //max read position is less than first ongoing transaction message position, so entryId -1
- maxReadPosition = PositionImpl.get(firstPosition.getLedgerId(), firstPosition.getEntryId() - 1);
+ // max read position is less than first ongoing transaction message position, so entryId -1
Review Comment:
Please modify the notes here.
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "dao-jun (via GitHub)" <gi...@apache.org>.
dao-jun commented on code in PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#discussion_r1493800841
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -287,8 +287,8 @@ private void handleTransactionMessage(TxnID txnId, Position position) {
.checkAbortedTransaction(txnId)) {
ongoingTxns.put(txnId, (PositionImpl) position);
PositionImpl firstPosition = ongoingTxns.get(ongoingTxns.firstKey());
- //max read position is less than first ongoing transaction message position, so entryId -1
- maxReadPosition = PositionImpl.get(firstPosition.getLedgerId(), firstPosition.getEntryId() - 1);
+ // max read position is less than first ongoing transaction message position
+ maxReadPosition = getPreviousPosition(firstPosition);
Review Comment:
In fact, `entryId=-1` does not cause program exceptions, There are many uses of this in pulsar.
If modified here, it's not necessary but also acceptable
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#discussion_r1493924929
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -287,8 +287,8 @@ private void handleTransactionMessage(TxnID txnId, Position position) {
.checkAbortedTransaction(txnId)) {
ongoingTxns.put(txnId, (PositionImpl) position);
PositionImpl firstPosition = ongoingTxns.get(ongoingTxns.firstKey());
- //max read position is less than first ongoing transaction message position, so entryId -1
- maxReadPosition = PositionImpl.get(firstPosition.getLedgerId(), firstPosition.getEntryId() - 1);
+ // max read position is less than first ongoing transaction message position
+ maxReadPosition = getPreviousPosition(firstPosition);
Review Comment:
There are two places that we need to get the previous position, but they are not consistent. We have better make them consistent.
https://github.com/apache/pulsar/blob/825e997216dabe23a6dde0945ef769bbda0558e4/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java#L290-L291
https://github.com/apache/pulsar/blob/825e997216dabe23a6dde0945ef769bbda0558e4/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java#L449-L450
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on code in PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#discussion_r1493924929
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java:
##########
@@ -287,8 +287,8 @@ private void handleTransactionMessage(TxnID txnId, Position position) {
.checkAbortedTransaction(txnId)) {
ongoingTxns.put(txnId, (PositionImpl) position);
PositionImpl firstPosition = ongoingTxns.get(ongoingTxns.firstKey());
- //max read position is less than first ongoing transaction message position, so entryId -1
- maxReadPosition = PositionImpl.get(firstPosition.getLedgerId(), firstPosition.getEntryId() - 1);
+ // max read position is less than first ongoing transaction message position
+ maxReadPosition = getPreviousPosition(firstPosition);
Review Comment:
There are two places that we need to get the previous position, but they are not consistent. We need to make them consistent.
https://github.com/apache/pulsar/blob/825e997216dabe23a6dde0945ef769bbda0558e4/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java#L290-L291
https://github.com/apache/pulsar/blob/825e997216dabe23a6dde0945ef769bbda0558e4/pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java#L449-L450
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1956009864
/pulsarbot rerun-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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "thetumbled (via GitHub)" <gi...@apache.org>.
thetumbled commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1956604878
Hi, I have add a test to produce a exception, and it is necessary to fix this problem.
PTAL, thanks. @dao-jun @congbobo184 @liangyepianzhou
--
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
Re: [PR] [fix] [txn] Get previous position by managed ledger. [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #22024:
URL: https://github.com/apache/pulsar/pull/22024#issuecomment-1965145123
> 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 ?
--
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