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

Ledger replicate supports throttle #2778

Merged
merged 2 commits into from
Dec 13, 2021
Merged

Ledger replicate supports throttle #2778

merged 2 commits into from
Dec 13, 2021

Conversation

gaozhangmin
Copy link
Contributor

@gaozhangmin gaozhangmin commented Aug 27, 2021 •
edited
Loading

Motivation

Ledger replicating puts heavy loads on cluster.
Now, ledger replicate only supports split fragments into small pieces.
But, throttling is not supported.

Changes

Add a confiuration replicationRateByBytes

support throttling read rate in bytes.

Also bookkeeper shell recover command supports throttle.

@gaozhangmin gaozhangmin changed the title Throttle replicate rate in bytes Ledger replicate supports throttle Aug 27, 2021
@gaozhangmin gaozhangmin marked this pull request as draft August 30, 2021 05:22
@gaozhangmin gaozhangmin marked this pull request as ready for review August 30, 2021 16:19
@gaozhangmin
Copy link
Contributor Author

gaozhangmin commented Aug 31, 2021 •
edited
Loading

below is the throttle results. with replicationRateByBytes = 50MB
image

@eolivelli @hangc0276 PTAL

eolivelli
eolivelli requested changes Aug 31, 2021
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.

Overall I support this patch.
I wonder if it is possible to not add new constructors and pass ServerConfiguration down to the various components.

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java Outdated Show resolved Hide resolved
@gaozhangmin
Copy link
Contributor Author

@eolivelli PTAL, thanks

@gaozhangmin gaozhangmin force-pushed the replication-throttle branch 3 times, most recently from 8c3ae00 to 60305e2 Compare September 16, 2021 04:56
@gaozhangmin
Copy link
Contributor Author

@dlg99 @hangc0276 @nicoloboschi PTAL

hangc0276
hangc0276 approved these changes Nov 29, 2021
View reviewed changes
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.

Great work!

bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java Show resolved Hide resolved
@hangc0276
Copy link
Contributor

@gaozhangmin You'd better use one parameter to control read and write throttle by bytes, which will simplify the configuration for users.

@hangc0276
Copy link
Contributor

@gaozhangmin You can just use read throttle by bytes to control replication, maybe no need write throttle.

@gaozhangmin gaozhangmin force-pushed the replication-throttle branch 4 times, most recently from 463bc15 to 3a16ee7 Compare December 3, 2021 09:50
@gaozhangmin
Copy link
Contributor Author

@hangc0276 @eolivelli @zymap PTAL again

eolivelli
eolivelli approved these changes Dec 3, 2021
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.

LGTM

zymap
zymap reviewed Dec 6, 2021
View reviewed changes
Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Left some minor comments.

...er-server/src/main/java/org/apache/bookkeeper/tools/cli/commands/bookies/RecoverCommand.java Outdated Show resolved Hide resolved
nicoloboschi
nicoloboschi requested changes Dec 6, 2021
View reviewed changes
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java Outdated Show resolved Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java Show resolved Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java Outdated Show resolved Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java Outdated Show resolved Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java Outdated Show resolved Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java Show resolved Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java Outdated Show resolved Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java Outdated Show resolved Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java Outdated Show resolved Hide resolved
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java Show resolved Hide resolved
@gaozhangmin
Copy link
Contributor Author

@hangc0276 @zymap @nicoloboschi PTAL again, Thx.

@gaozhangmin
Copy link
Contributor Author

image
image

throttle results updated as above replicationRateByBytes = 50MB

hangc0276
hangc0276 reviewed Dec 7, 2021
View reviewed changes
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.

Great job!

@hangc0276
Copy link
Contributor

rerun-failure-tests

@zymap
Copy link
Member

zymap commented Dec 7, 2021

rerun failure checks

@zymap
Copy link
Member

zymap commented Dec 13, 2021

@gaozhangmin Could you please take a look at the conflicts? I want to merge this PR

@gaozhangmin
Copy link
Contributor Author

@gaozhangmin Could you please take a look at the conflicts? I want to merge this PR

done.

@gaozhangmin
Copy link
Contributor Author

@zymap

@zymap zymap merged commit a2d7341 into apache:master Dec 13, 2021
@StevenLuMT StevenLuMT mentioned this pull request Jan 14, 2022
Merged
zymap pushed a commit that referenced this pull request Jan 19, 2022
@StevenLuMT
improve the throttle function (#2991)
7f31748 
Descriptions of the changes in this PR:


### Motivation

improve the throttle function : old pr: #2778 
1. duplicate definition for replicationRateByBytes
2.make sure this update safety when different callback run averageEntrySize updating

### Changes

1.clean code for duplicate definition for replicationRateByBytes
2.add a lock to make sure data safety for updateAverageEntrySize
@zymap zymap added this to the 4.16.0 milestone Apr 27, 2022
zymap pushed a commit that referenced this pull request Aug 1, 2022
@gaozhangmin @zymap
Ledger replicate supports throttle (#2778)
688460b 
Ledger replicating puts  heavy loads on cluster.
Now,  ledger replicate only supports split fragments into small pieces.
 But, throttling is not supported.

Add a confiuration `replicationRateByBytes `

support throttling  read rate in bytes.

Also bookkeeper shell recover command supports throttle.

(cherry picked from commit a2d7341)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 5, 2022
@gaozhangmin @hangc0276
Ledger replicate supports throttle (apache#2778)
6992dfc 
### Motivation

Ledger replicating puts  heavy loads on cluster.
Now,  ledger replicate only supports split fragments into small pieces.
 But, throttling is not supported.

### Changes

Add a confiuration `replicationRateByBytes `

support throttling  read rate in bytes.

Also bookkeeper shell recover command supports throttle.

(cherry picked from commit a2d7341)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 5, 2022
@StevenLuMT @hangc0276
improve the throttle function (apache#2991)
b3c8f7f 
Descriptions of the changes in this PR:

improve the throttle function : old pr: apache#2778
1. duplicate definition for replicationRateByBytes
2.make sure this update safety when different callback run averageEntrySize updating

1.clean code for duplicate definition for replicationRateByBytes
2.add a lock to make sure data safety for updateAverageEntrySize

(cherry picked from commit 7f31748)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
@gaozhangmin @hangc0276
Ledger replicate supports throttle (apache#2778)
7267058 
### Motivation

Ledger replicating puts  heavy loads on cluster.
Now,  ledger replicate only supports split fragments into small pieces.
 But, throttling is not supported.

### Changes

Add a confiuration `replicationRateByBytes `

support throttling  read rate in bytes.

Also bookkeeper shell recover command supports throttle.

(cherry picked from commit a2d7341)
hangc0276 pushed a commit to hangc0276/bookkeeper that referenced this pull request Nov 7, 2022
@StevenLuMT @hangc0276
improve the throttle function (apache#2991)
a13dfce 
Descriptions of the changes in this PR:

improve the throttle function : old pr: apache#2778
1. duplicate definition for replicationRateByBytes
2.make sure this update safety when different callback run averageEntrySize updating

1.clean code for duplicate definition for replicationRateByBytes
2.add a lock to make sure data safety for updateAverageEntrySize

(cherry picked from commit 7f31748)
nicoloboschi pushed a commit to datastax/bookkeeper that referenced this pull request Jan 11, 2023
@gaozhangmin @nicoloboschi
Ledger replicate supports throttle (apache#2778)
40c3ecc 
### Motivation

Ledger replicating puts  heavy loads on cluster.
Now,  ledger replicate only supports split fragments into small pieces.
 But, throttling is not supported.

### Changes

Add a confiuration `replicationRateByBytes `

support throttling  read rate in bytes.

Also bookkeeper shell recover command supports throttle.

(cherry picked from commit a2d7341)
(cherry picked from commit 7267058)
nicoloboschi pushed a commit to datastax/bookkeeper that referenced this pull request Jan 11, 2023
@StevenLuMT @nicoloboschi
improve the throttle function (apache#2991)
16b97d5 
Descriptions of the changes in this PR:

improve the throttle function : old pr: apache#2778
1. duplicate definition for replicationRateByBytes
2.make sure this update safety when different callback run averageEntrySize updating

1.clean code for duplicate definition for replicationRateByBytes
2.add a lock to make sure data safety for updateAverageEntrySize

(cherry picked from commit 7f31748)
(cherry picked from commit a13dfce)
@wenbingshen wenbingshen mentioned this pull request Apr 28, 2024
Merged
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

@nicoloboschi nicoloboschi nicoloboschi approved these changes

@zymap zymap zymap approved these changes

Assignees

@gaozhangmin gaozhangmin

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@gaozhangmin @hangc0276 @zymap @eolivelli @nicoloboschi