Skip to content
This repository was archived by the owner on Jan 24, 2024. It is now read-only.

Prevent incomplete future and add more logs for ProducerIdManager - #1391

Merged
BewareMyPower merged 1 commit into
streamnative:masterfrom
BewareMyPower:bewaremypower/init-pid-enhance
Jul 7, 2022
Merged

Prevent incomplete future and add more logs for ProducerIdManager#1391
BewareMyPower merged 1 commit into
streamnative:masterfrom
BewareMyPower:bewaremypower/init-pid-enhance

Conversation

@BewareMyPower

@BewareMyPower BewareMyPower commented Jul 7, 2022

Copy link
Copy Markdown
Contributor

Motivation

Currently, if the ProducerIdManager failed to generate the producer ID,
the future of epoch and TxnMetadata would never complete, see

kop/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java

Lines 248 to 253 in f63b191

if (throwable != null) {
responseCallback.accept(new InitProducerIdResult(
-1L,
producerEpoch
, Errors.UNKNOWN_SERVER_ERROR));
return;

epochAndTxnMetaFuture would not complete in the case above.

In addition, since epochAndTxnMetaFuture never complete exceptionally,
the following code is redundant:

kop/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java

Lines 304 to 309 in f63b191

}).exceptionally(ex -> {
log.error("Get epoch and TxnMetadata failed.", ex);
responseCallback.accept(initTransactionError(Errors.forException(ex.getCause())));
return null;
});

Modifications

When the ProducerIdManager failed to generate the producer ID,
complete epochAndTxnMetaFuture with null and handle the null value in
the callback.

Then add some debug and error logs for ProducerIdManager. Regarding
the exceptionally block of epochAndTxnMetaFuture, treat it as an
unexpected error caused by thenAccept block and return a
UNKNOWN_SERVER_ERROR as well. Because in this case
Errors.forException(ex.getCause()) never return a valid error code.

Documentation

Check the box below.

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@BewareMyPower
Prevent incomplete future and add more logs for ProducerIdManager
868994f 
### Motivation

Currently, if the `ProducerIdManager` failed to generate the producer ID,
the future of epoch and `TxnMetadata` would never complete, see

https://github.com/streamnative/kop/blob/f63b191e72d2319f67f7a5ab7242e898c10ed9c5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java#L248-L253

`epochAndTxnMetaFuture` would not complete in the case above.

In addition, since `epochAndTxnMetaFuture` never complete exceptionally,
the following code is redundant:

https://github.com/streamnative/kop/blob/f63b191e72d2319f67f7a5ab7242e898c10ed9c5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java#L304-L309

### Modifications

When the `ProducerIdManager` failed to generate the producer ID,
complete `epochAndTxnMetaFuture` with null and handle the null value in
the callback.

Then add some debug and error logs for `ProducerIdManager`. Regarding
the `exceptionally` block of `epochAndTxnMetaFuture`, treat it as an
unexpected error caused by `thenAccept` block and return a
`UNKNOWN_SERVER_ERROR` as well. Because in this case
`Errors.forException(ex.getCause())` never return a valid error code.
@BewareMyPower
BewareMyPower requested review from a team and jiazhai as code owners July 7, 2022 13:02
@BewareMyPower BewareMyPower self-assigned this Jul 7, 2022
@github-actions github-actions Bot added the no-need-doc This pr does not need any document label Jul 7, 2022
Demogorgon314
Demogorgon314 approved these changes Jul 7, 2022
View reviewed changes

@Demogorgon314 Demogorgon314 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@BewareMyPower
BewareMyPower merged commit dc9d49a into streamnative:master Jul 7, 2022
@BewareMyPower
BewareMyPower deleted the bewaremypower/init-pid-enhance branch July 7, 2022 13:54
BewareMyPower added a commit that referenced this pull request Jul 9, 2022
@BewareMyPower
Prevent incomplete future and add more logs for ProducerIdManager (#1391
e6d4d10 
)

### Motivation

Currently, if the `ProducerIdManager` failed to generate the producer ID,
the future of epoch and `TxnMetadata` would never complete, see

https://github.com/streamnative/kop/blob/f63b191e72d2319f67f7a5ab7242e898c10ed9c5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java#L248-L253

`epochAndTxnMetaFuture` would not complete in the case above.

In addition, since `epochAndTxnMetaFuture` never complete exceptionally,
the following code is redundant:

https://github.com/streamnative/kop/blob/f63b191e72d2319f67f7a5ab7242e898c10ed9c5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java#L304-L309

### Modifications

When the `ProducerIdManager` failed to generate the producer ID,
complete `epochAndTxnMetaFuture` with null and handle the null value in
the callback.

Then add some debug and error logs for `ProducerIdManager`. Regarding
the `exceptionally` block of `epochAndTxnMetaFuture`, treat it as an
unexpected error caused by `thenAccept` block and return a
`UNKNOWN_SERVER_ERROR` as well. Because in this case
`Errors.forException(ex.getCause())` never return a valid error code.

(cherry picked from commit dc9d49a)
BewareMyPower added a commit that referenced this pull request Jul 19, 2022
@BewareMyPower
Prevent incomplete future and add more logs for ProducerIdManager (#1391
c167e04 
)

### Motivation

Currently, if the `ProducerIdManager` failed to generate the producer ID,
the future of epoch and `TxnMetadata` would never complete, see

https://github.com/streamnative/kop/blob/f63b191e72d2319f67f7a5ab7242e898c10ed9c5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java#L248-L253

`epochAndTxnMetaFuture` would not complete in the case above.

In addition, since `epochAndTxnMetaFuture` never complete exceptionally,
the following code is redundant:

https://github.com/streamnative/kop/blob/f63b191e72d2319f67f7a5ab7242e898c10ed9c5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java#L304-L309

### Modifications

When the `ProducerIdManager` failed to generate the producer ID,
complete `epochAndTxnMetaFuture` with null and handle the null value in
the callback.

Then add some debug and error logs for `ProducerIdManager`. Regarding
the `exceptionally` block of `epochAndTxnMetaFuture`, treat it as an
unexpected error caused by `thenAccept` block and return a
`UNKNOWN_SERVER_ERROR` as well. Because in this case
`Errors.forException(ex.getCause())` never return a valid error code.

(cherry picked from commit dc9d49a)
BewareMyPower added a commit that referenced this pull request Jul 20, 2022
@BewareMyPower
Prevent incomplete future and add more logs for ProducerIdManager (#1391
4d59019 
)

### Motivation

Currently, if the `ProducerIdManager` failed to generate the producer ID,
the future of epoch and `TxnMetadata` would never complete, see

https://github.com/streamnative/kop/blob/f63b191e72d2319f67f7a5ab7242e898c10ed9c5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java#L248-L253

`epochAndTxnMetaFuture` would not complete in the case above.

In addition, since `epochAndTxnMetaFuture` never complete exceptionally,
the following code is redundant:

https://github.com/streamnative/kop/blob/f63b191e72d2319f67f7a5ab7242e898c10ed9c5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java#L304-L309

### Modifications

When the `ProducerIdManager` failed to generate the producer ID,
complete `epochAndTxnMetaFuture` with null and handle the null value in
the callback.

Then add some debug and error logs for `ProducerIdManager`. Regarding
the `exceptionally` block of `epochAndTxnMetaFuture`, treat it as an
unexpected error caused by `thenAccept` block and return a
`UNKNOWN_SERVER_ERROR` as well. Because in this case
`Errors.forException(ex.getCause())` never return a valid error code.

(cherry picked from commit dc9d49a)
michaeljmarshall pushed a commit to michaeljmarshall/kop that referenced this pull request Dec 13, 2022
@BewareMyPower @eolivelli
Prevent incomplete future and add more logs for ProducerIdManager (st…
2d7f6f0 
…reamnative#1391)

### Motivation

Currently, if the `ProducerIdManager` failed to generate the producer ID,
the future of epoch and `TxnMetadata` would never complete, see

https://github.com/streamnative/kop/blob/f63b191e72d2319f67f7a5ab7242e898c10ed9c5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java#L248-L253

`epochAndTxnMetaFuture` would not complete in the case above.

In addition, since `epochAndTxnMetaFuture` never complete exceptionally,
the following code is redundant:

https://github.com/streamnative/kop/blob/f63b191e72d2319f67f7a5ab7242e898c10ed9c5/kafka-impl/src/main/java/io/streamnative/pulsar/handlers/kop/coordinator/transaction/TransactionCoordinator.java#L304-L309

### Modifications

When the `ProducerIdManager` failed to generate the producer ID,
complete `epochAndTxnMetaFuture` with null and handle the null value in
the callback.

Then add some debug and error logs for `ProducerIdManager`. Regarding
the `exceptionally` block of `epochAndTxnMetaFuture`, treat it as an
unexpected error caused by `thenAccept` block and return a
`UNKNOWN_SERVER_ERROR` as well. Because in this case
`Errors.forException(ex.getCause())` never return a valid error code.

(cherry picked from commit dc9d49a)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jiazhai jiazhai Awaiting requested review from jiazhai
1 more reviewer
@Demogorgon314 Demogorgon314 Demogorgon314 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@BewareMyPower BewareMyPower

Labels

cherry-picked/branch-2.8.3 cherry-picked/branch-2.9.3 cherry-picked/branch-2.10.1 no-need-doc This pr does not need any document release/2.8.3 release/2.9.3 release/2.10.1 type/bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@BewareMyPower @Demogorgon314