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

PCIP-3 Pulsar Extended Transaction API Enhancement Proposal #11

Merged
merged 8 commits into from
Dec 17, 2024

Conversation

liangyepianzhou
Copy link
Contributor

Motivation

The motivation behind this proposal is to address the issue of duplicate message consumption when using Fail-over subscription mode with cumulative Ack in Pulsar. Despite the use of transactions, achieving exactly-once semantics has been problematic due to the inherent behavior of cumulative Ack. This issue is particularly challenging to resolve within the constraints of the Pulsar main repository for several reasons:

  • Complexity and Longevity: Fixing this issue without altering the Client API is complex and time-consuming.
  • Confusing API Usage: The existing transaction APIs in Pulsar are often confusing, with methods like abort() and commit() appearing synchronous but functioning asynchronously. This leads to incorrect usage patterns, such as needing to call abort().get() and commit().get() for proper operation.
  • API Modification Challenges: Modifying the Client API in the Pulsar main repository is difficult due to the uncertainty of whether a new solution will be perfect and the lengthy API update cycle.

Modifications

The proposed solution involves designing a new transaction API in the contributor repository that wraps the original Transaction API. This approach offers several benefits:

  • Concise Solution: By wrapping the original API, the problem can be solved in a straightforward and concise manner.
  • Best Practice Reference: This wrapping serves as a best practice example that does not disrupt existing users' usage while providing a reference solution for those encountering similar issues.
  • Enhanced Usability: The new API aims to clarify and optimize the transaction methods, making them more intuitive and less prone to misuse, thus improving the overall usability and clarity of transactional operations in Pulsar.

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.

(or)

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

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

@liangyepianzhou liangyepianzhou self-assigned this Dec 15, 2024
@eolivelli
Copy link

I think that it is definitively worth to push this initiative to the main repo

@liangyepianzhou
Copy link
Contributor Author

I think that it is definitively worth to push this initiative to the main repo

I understand your point that the initiative aims to address an existing bug in Pulsar, which indeed warrants consideration for integration into the main repository. However, modifying the public API in the Pulsar main repository is a challenging and lengthy process, and we cannot guarantee that the new API will be flawless and not require further modifications. Therefore, it makes sense to first iterate on the contributor repository, allowing some users to adopt it and solve their issues before considering a merge into the main repository.

@AuroraTwinkle
Copy link

The CI is failed.

@StevenLuMT
Copy link
Member

Change the format of the title. Do you need to change it to:

[improve][pcip] PCIP-3 Pulsar Extended Transaction API Enhancement Proposal

@liangyepianzhou liangyepianzhou changed the title PCIP-3:Pulsar Extended Transaction API Enhancement Proposal PCIP-3 Pulsar Extended Transaction API Enhancement Proposal Dec 16, 2024
@AuroraTwinkle
Copy link

LGTM

Copy link

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1 to keep this here for now

@liangyepianzhou liangyepianzhou merged commit 892df92 into apache:main Dec 17, 2024
2 checks passed
@liangyepianzhou liangyepianzhou deleted the txn_api branch December 17, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants