Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cherry-pick] [branch-3.0] Fix getting last message ID when there are ongoing transactions #22130

Closed
wants to merge 3 commits into from

Conversation

thetumbled
Copy link
Member

@thetumbled thetumbled commented Feb 27, 2024

Motivation

Exception is found in branch 3.0:
#22024 (comment)

Modifications

The root is that we do not cherry pick #21466 into branch 3.0, and the test code rely on this pr.
Moreover, the test code in #21466 is merged into branch 3.0 along with my pr #22024, so we do not need to cherry pick those test code in this pr.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is already covered by existing tests, such as (please describe tests).

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

@thetumbled thetumbled changed the base branch from master to branch-3.0 February 27, 2024 09:07
Copy link

@thetumbled Please add the following content to your PR description and select a checkbox:

- [ ] `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 -->

@thetumbled
Copy link
Member Author

PTAL, thanks.
@lhotari @Technoboy-

@lhotari
Copy link
Member

lhotari commented Feb 27, 2024

@Technoboy- I think we should cherry-pick #21466 first , that will ease tracking in the future

Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's first cherry-pick #21466

@thetumbled thetumbled changed the title [fix] [branch-3.0] Fix exception in TopicTransactionBufferTest [cherry-pick] [branch-3.0] Fix getting last message ID when there are ongoing transactions Feb 27, 2024
@thetumbled
Copy link
Member Author

let's first cherry-pick #21466

may be change the title is enough?

@Technoboy-
Copy link
Contributor

@Technoboy- I think we should cherry-pick #21466 first , that will ease tracking in the future

Can't cherry-pick directly. Using this and comment on the #21466 is enough

@lhotari
Copy link
Member

lhotari commented Feb 28, 2024

Closing and reopening as an attempt to trigger the CI builds to run. For some reason, the build didn't get triggered.

@lhotari lhotari closed this Feb 28, 2024
@lhotari lhotari reopened this Feb 28, 2024
@thetumbled
Copy link
Member Author

thetumbled commented Feb 28, 2024

Some test do not pass:
mvn -B -T 1C -ntp -Pcore-modules,-main clean install -DskipTests -Dlicense.skip=true -Drat.skip=true -Dcheckstyle.skip=true

cannot find symbol
Error:    symbol:   class TimeUnitToSecondsConverter
Error:    location: class org.apache.pulsar.admin.cli.CmdNamespaces.SetOffloadThreshold
image

TimeUnitToSecondsConverter.class is introduced by #20782 , but we do not cherry pick it into branch 3.0.
However, #22101 is cherry picked into branch-3.0, which rely on TimeUnitToSecondsConverter.class.

So the solution is: first, cherry pick #20782 into branch-3.0, then we use this pr #22143 to fix the problem.
PTAL, thanks. @lhotari @Technoboy-

@thetumbled
Copy link
Member Author

Some test do not pass: mvn -B -T 1C -ntp -Pcore-modules,-main clean install -DskipTests -Dlicense.skip=true -Drat.skip=true -Dcheckstyle.skip=true

cannot find symbol
Error:    symbol:   class TimeUnitToSecondsConverter
Error:    location: class org.apache.pulsar.admin.cli.CmdNamespaces.SetOffloadThreshold
image TimeUnitToSecondsConverter.class is introduced by #20782 , but we do not cherry pick it into branch 3.0. However, #22101 is cherry picked into branch-3.0, which rely on TimeUnitToSecondsConverter.class.

So the solution is: first, cherry pick #20782 into branch-3.0, then we use this pr #22143 to fix the problem. PTAL, thanks. @lhotari @Technoboy-

The problem has been fixed by #22169, we can proceed with reviewing this pr. @lhotari @heesung-sn

@heesung-sn heesung-sn self-requested a review March 1, 2024 20:11
@heesung-sn
Copy link
Contributor

I don't see this change in this cherry-pick PR.

https://github.com/apache/pulsar/pull/21466/files#diff-ecd728301a585f256e8a649b5e65b28c166194477355b3a1eefc198d014c25d3R450

@thetumbled can you check again?

@thetumbled
Copy link
Member Author

thetumbled commented Mar 2, 2024

I don't see this change in this cherry-pick PR.

https://github.com/apache/pulsar/pull/21466/files#diff-ecd728301a585f256e8a649b5e65b28c166194477355b3a1eefc198d014c25d3R450

@thetumbled can you check again?

Sorry for missing it. PTAL, thanks.

@lhotari
Copy link
Member

lhotari commented Mar 4, 2024

Thank you @thetumbled for this PR. I ended up using your merge conflict resolution into this commit: 4489d03 .
I hope we find a way to fix our process that we don't end up in this problems that often in the future. I have started a discussion thread on the dev mailing list: https://lists.apache.org/thread/j6qvt45rndnvjypcmqxsfmddqt41bxjv

@lhotari lhotari closed this Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants