Skip to content

Conversation

@hangc0276
Copy link
Contributor

Motivation

#3837 introduced group flush add responses triggered by journal sync. However, if we skip writing journals, the add responses won't be flushed to the netty channel and the client will receive write entries timeout.

Changes

  • Flush the add responses when skipping writing journals
  • Add tests to cover V2 protocol and skip writing journal cases.

Copy link
Contributor

@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.

Great catch

It's weird that no test failed!

@hangc0276
Copy link
Contributor Author

@merlimat @eolivelli @zymap @horizonzy Please help take a look at this fix, and it is a blocker for BookKeeper 4.16.0 release.

@hangc0276
Copy link
Contributor Author

Great catch

It's weird that no test failed!

@eolivelli I found no tests covered this case.

Copy link
Member

@horizonzy horizonzy left a comment

Choose a reason for hiding this comment

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

LGTM

@hangc0276 hangc0276 merged commit 5deadcc into apache:master Mar 26, 2023
hangc0276 added a commit that referenced this pull request Mar 26, 2023
### Motivation
#3837 introduced group flush add responses triggered by journal sync. However, if we skip writing journals, the add responses won't be flushed to the netty channel and the client will receive write entries timeout.

### Changes
- Flush the add responses when skipping writing journals
- Add tests to cover V2 protocol and skip writing journal cases.

(cherry picked from commit 5deadcc)
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
### Motivation
apache#3837 introduced group flush add responses triggered by journal sync. However, if we skip writing journals, the add responses won't be flushed to the netty channel and the client will receive write entries timeout.

### Changes
- Flush the add responses when skipping writing journals
- Add tests to cover V2 protocol and skip writing journal cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants