Skip to content

Conversation

@yapxue
Copy link
Contributor

@yapxue yapxue commented Aug 23, 2022

Fix group ForceWrite not take effect with forceWriteMarkerSent in while loop of ForceWriteThread

Motivation

Bookkeeper 4.15 has performance issue. There are too many journal sync.
Then I found in ForceWriteThread, forceWriteMarkerSent is reset to false for every loop, and this can cause shouldForceWrite=true and trigger journal sync. This new logic is bringed by #2962

Changes

move forceWriteMarkerSent=false out of while loop.

Copy link
Member

@StevenLuMT StevenLuMT left a comment

Choose a reason for hiding this comment

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

great job

@StevenLuMT
Copy link
Member

StevenLuMT commented Aug 23, 2022

@dlg99 @zymap @hangc0276 @eolivelli have a look,thanks

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.

LGTM

@dlg99 you merged #2962
can you please take a look here?

@eolivelli eolivelli added this to the 4.16.0 milestone Aug 23, 2022
@eolivelli eolivelli requested a review from zymap August 23, 2022 11:06
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

LGTM

Is it able to add a test to avoid the regression in the future?

boolean shouldForceWrite = true;
int numReqInLastForceWrite = 0;
long busyStartTime = System.nanoTime();
boolean forceWriteMarkerSent = false;
Copy link
Member

Choose a reason for hiding this comment

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

I think line:563 affects judgment @zymap

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. I got it

@zymap
Copy link
Member

zymap commented Aug 23, 2022

Is it able to add a test to avoid the regression in the future?

+1

Copy link
Contributor

@hangc0276 hangc0276 left a comment

Choose a reason for hiding this comment

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

Nice catch!

Copy link
Contributor

@dlg99 dlg99 left a comment

Choose a reason for hiding this comment

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

LGTM, good catch.
Definitely needs a test.

@StevenLuMT
Copy link
Member

@zymap @eolivelli this pr can be merged,
please cherry pick to release/4.15.1

@zymap zymap merged commit dcc9a41 into apache:master Aug 24, 2022
zymap pushed a commit that referenced this pull request Aug 24, 2022
…equently forceWrite (#3454)

Fix group ForceWrite not take effect with forceWriteMarkerSent in while loop of ForceWriteThread

### Motivation
Bookkeeper 4.15 has performance issue. There are too many journal sync.
Then I found in ForceWriteThread, forceWriteMarkerSent is reset to false for every loop, and this can cause shouldForceWrite=true and trigger journal sync. This new logic is bringed by #2962

### Changes
move forceWriteMarkerSent=false out of while loop.


(cherry picked from commit dcc9a41)
Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024
…equently forceWrite (apache#3454)

Fix group ForceWrite not take effect with forceWriteMarkerSent in while loop of ForceWriteThread

### Motivation
Bookkeeper 4.15 has performance issue. There are too many journal sync.
Then I found in ForceWriteThread, forceWriteMarkerSent is reset to false for every loop, and this can cause shouldForceWrite=true and trigger journal sync. This new logic is bringed by apache#2962

### Changes
move forceWriteMarkerSent=false out of while loop.
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.

8 participants