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.

Sign up for GitHub

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

Jump to bottom

fix(logstreams): complete future when any error during opening an appender #8038

Merged
2 commits merged into from
Oct 27, 2021
Merged

fix(logstreams): complete future when any error during opening an appender #8038

2 commits merged into from
Oct 27, 2021

Conversation

deepthidevaki
Copy link
Contributor

Description

Not all exceptions occuring during opening an appender was handled properly. In such cases the caller (StreamProcessor) waiting for the writer will wait for ever because the future is not completed. This commit adds a catch all exception to the openAppender which completes the future exceptionally.

Related issues

Related #7992

closes #

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/0.25) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • New content is added to the release announcement

@deepthidevaki
fix(logstreams): complete future when any error during opening an app…
56376ed 
…ender

Not all exceptions occuring during opening an appender was handled properly. In such cases the caller (StreamProcessor) waiting for the writer will wait for ever because the future is not completed. This commit adds a catch all exception to the openAppender which completes the futuer exceptionally.
Zelldon
Zelldon reviewed Oct 22, 2021
View reviewed changes
Copy link
Member

@Zelldon Zelldon left a comment

Choose a reason for hiding this comment

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

Thanks @deepthidevaki could we please add a test for this? Or do you see any blockers for it?

logstreams/src/main/java/io/camunda/zeebe/logstreams/impl/log/LogStreamImpl.java Show resolved Hide resolved
logstreams/src/main/java/io/camunda/zeebe/logstreams/impl/log/LogStreamImpl.java Show resolved Hide resolved
@deepthidevaki
Copy link
Contributor Author

@Zelldon I have added test for the case when there is an exception when trying to read the last position.

Zelldon
Zelldon approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@Zelldon Zelldon left a comment

Choose a reason for hiding this comment

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

Thanks @deepthidevaki just a small question on your test but in general LGTM

logstreams/src/test/java/io/camunda/zeebe/logstreams/log/LogStreamErrorTest.java
@Before
public void setup() {
logStream =
SyncLogStream.builder()
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we use the Sync if you in the test anyway use the async one? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Async LogStreamBuilder can only be called from an actor, because it uses ActorFuture::onComplete. To handle that I have to add a wrapper code around it, which is already done in SyncLogStreamBuilder.

@deepthidevaki
Copy link
Contributor Author

bors r+

@ghost
Copy link

ghost commented Oct 27, 2021

Build succeeded:

@ghost ghost merged commit 9b455d3 into develop Oct 27, 2021
@ghost ghost deleted the dd-7992-error-handling branch October 27, 2021 12:01
@github-actions
Copy link
Contributor

Backport failed for stable/1.1, because 1 or more of the commits are not available.

Please cherry-pick the changes locally.
Note that rebase and squash merges are not supported at this time.
For more information see korthout/backport-action#46.

@github-actions
Copy link
Contributor

Backport failed for stable/1.2, because 1 or more of the commits are not available.

Please cherry-pick the changes locally.
Note that rebase and squash merges are not supported at this time.
For more information see korthout/backport-action#46.

@korthout korthout mentioned this pull request Oct 27, 2021
Closed
@korthout
Copy link
Member

@deepthidevaki I've fixed the bug above. Let's try to backport again, once #8084 is merged.

@deepthidevaki
Copy link
Contributor Author

Thanks @korthout

@korthout
Copy link
Member

@deepthidevaki should be possible again now. Feel free to try it out

@deepthidevaki
Copy link
Contributor Author

/backport

@github-actions github-actions bot mentioned this pull request Oct 27, 2021
Merged
@github-actions
Copy link
Contributor

Successfully created backport PR #8086 for stable/1.1.

@github-actions github-actions bot mentioned this pull request Oct 27, 2021
Merged
@github-actions
Copy link
Contributor

Successfully created backport PR #8087 for stable/1.2.

ghost pushed a commit that referenced this pull request Oct 27, 2021
@deepthidevaki
merge: #8087
e6ef430 
8087: [Backport stable/1.2] fix(logstreams): complete future when any error during opening an appender r=deepthidevaki a=github-actions[bot]

# Description
Backport of #8038 to `stable/1.2`.

relates to #7992

Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
ghost pushed a commit that referenced this pull request Oct 27, 2021
@deepthidevaki
merge: #8086
02a3405 
8086: [Backport stable/1.1] fix(logstreams): complete future when any error during opening an appender r=deepthidevaki a=github-actions[bot]

# Description
Backport of #8038 to `stable/1.1`.

relates to #7992

Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
ghost pushed a commit that referenced this pull request Oct 28, 2021
@deepthidevaki
merge: #8086
9bc66f3 
8086: [Backport stable/1.1] fix(logstreams): complete future when any error during opening an appender r=deepthidevaki a=github-actions[bot]

# Description
Backport of #8038 to `stable/1.1`.

relates to #7992

Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
ghost pushed a commit that referenced this pull request Oct 28, 2021
@deepthidevaki
merge: #8086
ea6a46c 
8086: [Backport stable/1.1] fix(logstreams): complete future when any error during opening an appender r=deepthidevaki a=github-actions[bot]

# Description
Backport of #8038 to `stable/1.1`.

relates to #7992

Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
ghost pushed a commit that referenced this pull request Oct 29, 2021
@deepthidevaki
merge: #8086
921908a 
8086: [Backport stable/1.1] fix(logstreams): complete future when any error during opening an appender r=Zelldon a=github-actions[bot]

# Description
Backport of #8038 to `stable/1.1`.

relates to #7992

Co-authored-by: Deepthi Devaki Akkoorath <deepthidevaki@gmail.com>
@deepthidevaki deepthidevaki mentioned this pull request Nov 9, 2021
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zelldon Zelldon Zelldon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@deepthidevaki @korthout @Zelldon @npepinpe @menski