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

The problem of two exception handling #12744

Merged
merged 2 commits into from
Nov 18, 2021
Merged

The problem of two exception handling #12744

merged 2 commits into from
Nov 18, 2021

Conversation

liangyepianzhou
Copy link
Contributor

@liangyepianzhou liangyepianzhou commented Nov 11, 2021

Motivation

The exception here was handled twice, resulting in a null pointer exception.
And the position will be updated twice.

Modifications

Remove exceptionally in this case.

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

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): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below and label this PR (if you have committer privilege).

Need to update docs?

  • no-need-doc

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Nov 11, 2021
@eolivelli
Copy link
Contributor

Do you have any error/stacktrace to demonstrate your case?

@liangyepianzhou
Copy link
Contributor Author

liangyepianzhou commented Nov 11, 2021

@eolivelli I encountered this problem when writing the offload of Transaction. Because there may be data that does not need to be transferred during offload, there may be holes when it is read out. In this regard, our processing is to return a NoSuchEntryException. Then we encountered this situation.Thanks to the help from @congbobo184 , this problem can be discovered.
#12449

@congbobo184 congbobo184 merged commit fc8d50e into apache:master Nov 18, 2021
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Nov 18, 2021
* up/master:
  Update deploy-bare-metal.md (apache#12432)
  [Broker] Fix producer getting incorrectly removed from topic's producers map (apache#12846)
  Add error log when new jetty client (apache#12840)
  JavaInstanceTest should be AssertEquals (apache#12836)
  [Transaction] Fix transaction flaky test testMaxReadPositionForNormalPublish (apache#12681)
  The problem of two exception handling (apache#12744)
  Fix TopicPoliciesCacheNotInitException issue. (apache#12773)
  Added local filesystem backend for package manager (apache#12708)
  [Java Client] Make userProvidedProducerName final (apache#12849)
  optimize indention in ServerCnx#handleProducer (apache#12854)
zeo1995 pushed a commit to zeo1995/pulsar that referenced this pull request Nov 18, 2021
* up/master:
  [Issue 12757][broker] add broker config isAllowAutoUpdateSchema (apache#12786)
  Update deploy-bare-metal.md (apache#12432)
  [Broker] Fix producer getting incorrectly removed from topic's producers map (apache#12846)
  Add error log when new jetty client (apache#12840)
  JavaInstanceTest should be AssertEquals (apache#12836)
  [Transaction] Fix transaction flaky test testMaxReadPositionForNormalPublish (apache#12681)
  The problem of two exception handling (apache#12744)
  Fix TopicPoliciesCacheNotInitException issue. (apache#12773)
  Added local filesystem backend for package manager (apache#12708)
@lhotari
Copy link
Member

lhotari commented Nov 18, 2021

@merlimat Are you ok with this change?

@lhotari lhotari requested a review from merlimat November 18, 2021 17:23
@lhotari
Copy link
Member

lhotari commented Nov 18, 2021

@congbobo184 This PR was reviewed and merged only by you. On the other hand, our contributing guidelines state that each PR must be approved by at least 2 Pulsar committers.
Was it intentional that you merged this PR?

lhotari added a commit that referenced this pull request Nov 18, 2021
@merlimat
Copy link
Contributor

@liangyepianzhou @congbobo184 I think the PR solves the problem but it has a regression.

The exceptionally() clause is also use to catch exceptions that are being thrown in the whenComplete() part. For example we have a checkNotNull(...) part there.

Instead, I'd suggest to keep the exceptionally() part and convert the whenComplete() into thenAccept().

Eg:

CompletableFuture<Void> f;

doSomething()
    .thenAccept(res -> {
         //// Potentially throw exception
        f.complete(..);
    }).exceptionally(ex -> {
        f.completeExceptionally(ex);
       return null;
    });

@liangyepianzhou liangyepianzhou mentioned this pull request Nov 19, 2021
2 tasks
codelipenghui pushed a commit that referenced this pull request Nov 19, 2021
Fixes #12744

### Motivation
The exception here was handled twice, resulting in a null pointer exception.
And the position will be updated twice.
### Modifications
Keep the exceptionally() part and convert the whenComplete() into thenAccept().
dlg99 pushed a commit to dlg99/pulsar that referenced this pull request Nov 23, 2021
whenCompleteAsync has handle exception, don't use exceptionally, otherwise it will be handle twice
dlg99 pushed a commit to dlg99/pulsar that referenced this pull request Nov 23, 2021
Fixes apache#12744

### Motivation
The exception here was handled twice, resulting in a null pointer exception.
And the position will be updated twice.
### Modifications
Keep the exceptionally() part and convert the whenComplete() into thenAccept().
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
whenCompleteAsync has handle exception, don't use exceptionally, otherwise it will be handle twice
eolivelli pushed a commit to eolivelli/pulsar that referenced this pull request Nov 29, 2021
Fixes apache#12744

### Motivation
The exception here was handled twice, resulting in a null pointer exception.
And the position will be updated twice.
### Modifications
Keep the exceptionally() part and convert the whenComplete() into thenAccept().
@codelipenghui codelipenghui added this to the 2.10.0 milestone Dec 17, 2021
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
whenCompleteAsync has handle exception, don't use exceptionally, otherwise it will be handle twice
fxbing pushed a commit to fxbing/pulsar that referenced this pull request Dec 19, 2021
Fixes apache#12744

### Motivation
The exception here was handled twice, resulting in a null pointer exception.
And the position will be updated twice.
### Modifications
Keep the exceptionally() part and convert the whenComplete() into thenAccept().
codelipenghui pushed a commit that referenced this pull request Dec 21, 2021
whenCompleteAsync has handle exception, don't use exceptionally, otherwise it will be handle twice

(cherry picked from commit fc8d50e)
codelipenghui pushed a commit that referenced this pull request Dec 21, 2021
Fixes #12744

### Motivation
The exception here was handled twice, resulting in a null pointer exception.
And the position will be updated twice.
### Modifications
Keep the exceptionally() part and convert the whenComplete() into thenAccept().

(cherry picked from commit dbac121)
@codelipenghui codelipenghui added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.9 Archived: 2.9 is end of life doc-not-needed Your PR changes do not impact docs release/2.9.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants