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

[improve][txn] Add getState in transaction for client API #17423

Conversation

congbobo184
Copy link
Contributor

@congbobo184 congbobo184 commented Sep 2, 2022

Motivation

now org.apache.pulsar.client.api.transaction.Transaction dont have a interface for user to get the transaction state.

user can get the transaction state to do user's own op.

Modifications

  1. add the interface in org.apache.pulsar.client.api.transaction.Transaction getState
  2. TransactionImpl implement the interface
     * Get transaction state.
     *
     * @return {@link State} the state of the transaction.
     */
    State getState();

Verifying this change

add the test

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • Does this pull request introduce a new feature? (yes)

  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

  • If a feature is not applicable for documentation, explain why?

  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

  • doc-not-needed

@congbobo184 congbobo184 added area/transaction doc-not-needed Your PR changes do not impact docs labels Sep 2, 2022
@congbobo184 congbobo184 added this to the 2.12.0 milestone Sep 2, 2022
@congbobo184 congbobo184 self-assigned this Sep 2, 2022
@codelipenghui codelipenghui added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Sep 2, 2022
@codelipenghui codelipenghui changed the title [feat][txn] Add getState in transaction for client API [improve][txn] Add getState in transaction for client API Sep 2, 2022
@codelipenghui
Copy link
Contributor

I hope it can be cherry-picked to release branches from branch-2.9.
So that users can use the new method to check the transaction state.
And the change will not introduce risk to release branches.

@codelipenghui
Copy link
Contributor

@Anonymitaet Please help review the Java Doc

Copy link
Member

@Anonymitaet Anonymitaet left a comment

Choose a reason for hiding this comment

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

Feel free to correct me if my understanding is inaccurate, thanks!

@Anonymitaet Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-not-needed Your PR changes do not impact docs labels Sep 5, 2022
congbobo184 and others added 3 commits September 5, 2022 21:08
…ransaction/Transaction.java

Co-authored-by: Anonymitaet <50226895+Anonymitaet@users.noreply.github.com>
…state_interface' into congbobo184_txn_add_transaction_state_interface
@congbobo184
Copy link
Contributor Author

@Anonymitaet @codelipenghui I have fixed the comments, please review again. Thanks! :)

Copy link
Member

@Anonymitaet Anonymitaet left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Approved from a technical writing perspective. 😊

Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

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

@congbobo184 would you mind to explain a case where the user needs to know the state of the transaction object? could you explain what are the actions they can take based on the state?

I'm ok with the pull itself but since we're adding it to the public API (and maintain it forever even if the internal transaction mechanism changes) we need a strong reason

@codelipenghui
Copy link
Contributor

@nicoloboschi I see a transaction production user. They are using TransactionImpl.checkIfOpen() to do some checks when they using transactions. Otherwise, they can only do it by catching exceptions, but it's a bad experience. The ideal case is users don't need to care about the transaction state, just commit it or abort it. Always under some complex use cases, users depend on the state check (same as client.isConnected(); consumer.isConnected();)

@congbobo184 congbobo184 reopened this Sep 21, 2022
@congbobo184 congbobo184 merged commit a9531db into apache:master Sep 22, 2022
@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 8, 2022
congbobo184 added a commit that referenced this pull request Nov 8, 2022
### Motivation
now `org.apache.pulsar.client.api.transaction.Transaction` dont have a interface for user to get the transaction state.

user can get the transaction state to do user's own op.
### Modifications
1. add the interface in `org.apache.pulsar.client.api.transaction.Transaction`     `getState`
2. TransactionImpl implement the interface
```
     * Get transaction state.
     *
     * @return {@link State} the state of the transaction.
     */
    State getState();
```
### Verifying this change
add the test

(cherry picked from commit a9531db)
congbobo184 added a commit that referenced this pull request Nov 30, 2022
### Motivation
now `org.apache.pulsar.client.api.transaction.Transaction` dont have a interface for user to get the transaction state.

user can get the transaction state to do user's own op.
### Modifications
1. add the interface in `org.apache.pulsar.client.api.transaction.Transaction`     `getState`
2. TransactionImpl implement the interface
```
     * Get transaction state.
     *
     * @return {@link State} the state of the transaction.
     */
    State getState();
```
### Verifying this change
add the test

(cherry picked from commit a9531db)
liangyepianzhou pushed a commit that referenced this pull request Dec 5, 2022
### Motivation
now `org.apache.pulsar.client.api.transaction.Transaction` dont have a interface for user to get the transaction state.

user can get the transaction state to do user's own op.
### Modifications
1. add the interface in `org.apache.pulsar.client.api.transaction.Transaction`     `getState`
2. TransactionImpl implement the interface
```
     * Get transaction state.
     *
     * @return {@link State} the state of the transaction.
     */
    State getState();
```
### Verifying this change
add the test

(cherry picked from commit a9531db)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Dec 6, 2022
### Motivation
now `org.apache.pulsar.client.api.transaction.Transaction` dont have a interface for user to get the transaction state.

user can get the transaction state to do user's own op.
### Modifications
1. add the interface in `org.apache.pulsar.client.api.transaction.Transaction`     `getState`
2. TransactionImpl implement the interface
```
     * Get transaction state.
     *
     * @return {@link State} the state of the transaction.
     */
    State getState();
```
### Verifying this change
add the test

(cherry picked from commit a9531db)
(cherry picked from commit db4fbfb)
liangyepianzhou pushed a commit that referenced this pull request Mar 16, 2023
now `org.apache.pulsar.client.api.transaction.Transaction` dont have a interface for user to get the transaction state.

user can get the transaction state to do user's own op.
1. add the interface in `org.apache.pulsar.client.api.transaction.Transaction`     `getState`
2. TransactionImpl implement the interface
```
     * Get transaction state.
     *
     * @return {@link State} the state of the transaction.
     */
    State getState();
```
add the test

(cherry picked from commit a9531db)
@Technoboy-
Copy link
Contributor

Cherry-picked by #19834

liangyepianzhou added a commit that referenced this pull request Mar 16, 2023
#17423) (#19834)

Co-authored-by: congbo <39078850+congbobo184@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/transaction cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/2.9.4 release/2.10.3 release/2.11.1 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants