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

Rename success with writableResult and update final writableResult about wait writeSet #3505

Merged
merged 8 commits into from Oct 19, 2022
Merged

Rename success with writableResult and update final writableResult about wait writeSet #3505

merged 8 commits into from Oct 19, 2022

Conversation

wenbingshen
Copy link
Member

Changes

update success about wait writeSet for Writable result.

@wenbingshen
Copy link
Member Author

ping @dlg99 @hangc0276 @shoothzj @eolivelli @StevenLuMT PTAL. Thanks.

zymap
zymap requested changes Sep 27, 2022
View reviewed changes
Copy link
Member

@zymap zymap left a comment •
edited

Choose a reason for hiding this comment

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

Great! Maybe we need to add a unit test?

@zymap zymap added this to the 4.16.0 milestone Sep 27, 2022
@wenbingshen
Copy link
Member Author

Great! Maybe we need to add a unit test?

@zymap I have add a unit test. PTAL. Thanks.

eolivelli
eolivelli approved these changes Sep 27, 2022
View reviewed changes
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.

Are you only renaming a variable?

I think that adding a test is the maim content of this patch. Maybe we should change the title

@wenbingshen wenbingshen changed the title update success about wait writeSet for Writable result Rename success with writableResult and update final writableResult about wait writeSet Sep 27, 2022
@wenbingshen
Copy link
Member Author

wenbingshen commented Sep 27, 2022 •
edited

@eolivelli Thanks for your comments.

Are you only renaming a variable?

No, I updated the title.

I think that adding a test is the maim content of this patch. Maybe we should change the title

The code before this PR is like this:

while (!isWriteSetWritable(writeSet, allowedNonWritableCount)) {
           ...     
}

The code after this PR is this:

while (!(writableResult = isWriteSetWritable(writeSet, allowedNonWritableCount))) {
           ...     
}

writableResult will not update the last result, but this result needs to be used later, we should update it.

StevenLuMT
StevenLuMT approved these changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

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

LGTM

@wenbingshen wenbingshen requested review from zymap and removed request for hangc0276 and dlg99 October 17, 2022 03:56
zymap
zymap reviewed Oct 18, 2022
View reviewed changes
Copy link
Member

@zymap zymap left a comment •
edited

Choose a reason for hiding this comment

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

Sorry for another minor comment. I just want to make sure the test can not be passed without this change.
I copied this test and run it locally, and it passed. I left a comment about how to fix it. PTAL. :)

bookkeeper-server/src/test/java/org/apache/bookkeeper/client/SlowBookieTest.java Outdated Show resolved Hide resolved
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/SlowBookieTest.java Outdated Show resolved Hide resolved
wenbingshen and others added 2 commits October 18, 2022 19:53
@wenbingshen @zymap
Update bookkeeper-server/src/test/java/org/apache/bookkeeper/client/S…
6f62cb5 
…lowBookieTest.java

Co-authored-by: Yong Zhang <zhangyong1025.zy@gmail.com>
@wenbingshen @zymap
Update bookkeeper-server/src/test/java/org/apache/bookkeeper/client/S…
c454d57 
…lowBookieTest.java

Co-authored-by: Yong Zhang <zhangyong1025.zy@gmail.com>
@wenbingshen
Copy link
Member Author

Sorry for another minor comment. I just want to make sure the test can not be passed without this change.
I copied this test and run it locally, and it passed. I left a comment about how to fix it. PTAL. :)

@zymap Good. Thanks for your comments. I have committed it. PTAL.

@wenbingshen
Copy link
Member Author

@zymap All tests passed. Can this pr be merged? Thanks.

@zymap zymap merged commit 2e1a2f0 into apache:master Oct 19, 2022
@wenbingshen wenbingshen deleted the updateSuccessFlag branch October 19, 2022 07:39
zymap pushed a commit that referenced this pull request Oct 26, 2022
@wenbingshen @zymap
Rename success with writableResult and update final writableResult ab…
d1f72b4 
…out wait writeSet (#3505)

### Changes
update `success` about wait writeSet for Writable result.

(cherry picked from commit 2e1a2f0)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
@wenbingshen @hangc0276
Rename success with writableResult and update final writableResult ab…
049e843 
…out wait writeSet (apache#3505)

### Changes
update `success` about wait writeSet for Writable result.

(cherry picked from commit 2e1a2f0)
@wenbingshen wenbingshen mentioned this pull request Nov 7, 2022
Merged
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
@wenbingshen @hangc0276
Rename success with writableResult and update final writableResult ab…
969bfc8 
…out wait writeSet (apache#3505)

### Changes
update `success` about wait writeSet for Writable result.

(cherry picked from commit 2e1a2f0)
nicoloboschi pushed a commit to datastax/bookkeeper that referenced this pull request Jan 11, 2023
@wenbingshen @nicoloboschi
Rename success with writableResult and update final writableResult ab…
ec553fc 
…out wait writeSet (apache#3505)

### Changes
update `success` about wait writeSet for Writable result.

(cherry picked from commit 2e1a2f0)
(cherry picked from commit 969bfc8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hangc0276 hangc0276 hangc0276 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@shoothzj shoothzj shoothzj approved these changes

@zymap zymap zymap approved these changes

@StevenLuMT StevenLuMT StevenLuMT approved these changes

Assignees

@wenbingshen wenbingshen

Labels
cherry-picked/branch-4.14 cherry-picked/branch-4.15 release/4.14.6 release/4.15.3 type/bug
Projects
None yet
Milestone
4.16.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wenbingshen @hangc0276 @eolivelli @shoothzj @zymap @StevenLuMT