You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@bookkeeper.apache.org by GitBox <gi...@apache.org> on 2021/12/06 07:01:14 UTC
[GitHub] [bookkeeper] nicoloboschi commented on a change in pull request #2778: Ledger replicate supports throttle
nicoloboschi commented on a change in pull request #2778:
URL: https://github.com/apache/bookkeeper/pull/2778#discussion_r762741217
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java
##########
@@ -112,10 +119,16 @@ public LedgerFragmentReplicator(BookKeeper bkc, StatsLogger statsLogger) {
numBytesWritten = this.statsLogger.getOpStatsLogger(NUM_BYTES_WRITTEN);
readDataLatency = this.statsLogger.getOpStatsLogger(READ_DATA_LATENCY);
writeDataLatency = this.statsLogger.getOpStatsLogger(WRITE_DATA_LATENCY);
+ if (conf.getReplicationRateByBytes() > 0) {
+ this.replicationThrottle = new Throttler(conf.getReplicationRateByBytes());
Review comment:
check positive arg
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java
##########
@@ -174,15 +174,21 @@ public BookKeeperAdmin(ClientConfiguration conf) throws IOException, Interrupted
* @param statsLogger
* - stats logger
*/
- public BookKeeperAdmin(final BookKeeper bkc, StatsLogger statsLogger) {
+ public BookKeeperAdmin(final BookKeeper bkc, StatsLogger statsLogger, ClientConfiguration conf) {
Review comment:
I'd like to add requireNonNull in for the ClientConfiguration to ensure we found out and replaced all the `BookKeeperAdmin ` constructor in the codebase
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
##########
@@ -210,6 +210,7 @@
"auditorMaxNumberOfConcurrentOpenLedgerOperations";
protected static final String AUDITOR_ACQUIRE_CONCURRENT_OPEN_LEDGER_OPERATIONS_TIMEOUT_MSEC =
"auditorAcquireConcurrentOpenLedgerOperationsTimeOutMSec";
+ protected static final String REPLICATION_RATE_BY_BYTES = "replicationRateByBytes";
Review comment:
isn't it a ClientConfiguration ? why we need the property also on ServerConfiguration ?
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java
##########
@@ -379,10 +397,17 @@ public void readComplete(int rc, LedgerHandle lh,
final long dataLength = data.length;
numEntriesRead.inc();
numBytesRead.registerSuccessfulValue(dataLength);
+ ++replicatedEntriesNum;
+
ByteBufList toSend = lh.getDigestManager()
.computeDigestAndPackageForSending(entryId,
lh.getLastAddConfirmed(), entry.getLength(),
Unpooled.wrappedBuffer(data, 0, data.length));
+ int toSendSize = toSend.readableBytes();
Review comment:
skip this part if replicationThrottle == null ?
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java
##########
@@ -112,10 +119,16 @@ public LedgerFragmentReplicator(BookKeeper bkc, StatsLogger statsLogger) {
numBytesWritten = this.statsLogger.getOpStatsLogger(NUM_BYTES_WRITTEN);
readDataLatency = this.statsLogger.getOpStatsLogger(READ_DATA_LATENCY);
writeDataLatency = this.statsLogger.getOpStatsLogger(WRITE_DATA_LATENCY);
+ if (conf.getReplicationRateByBytes() > 0) {
+ this.replicationThrottle = new Throttler(conf.getReplicationRateByBytes());
+ }
+ totalReplicatedEntriesSize = 0;
+ replicatedEntriesNum = 0;
+ averageEntrySize = 1024;
Review comment:
is it a magic number or is there a reason to be 1024 ?
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieShell.java
##########
@@ -482,6 +482,7 @@ public RecoverCmd() {
opts.addOption("sk", "skipOpenLedgers", false, "Skip recovering open ledgers");
opts.addOption("d", "deleteCookie", false, "Delete cookie node for the bookie.");
opts.addOption("sku", "skipUnrecoverableLedgers", false, "Skip unrecoverable ledgers.");
+ opts.addOption("trb", "replicationRateByBytes", false, "Replication rate by bytes");
Review comment:
we should have the short and long names consistent, `trb` talks about throttling (I suppose) but it should talk about replicationRateByBytes
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
##########
@@ -200,6 +200,29 @@
protected static final String CLIENT_CONNECT_BOOKIE_UNAVAILABLE_LOG_THROTTLING =
"clientConnectBookieUnavailableLogThrottling";
+ protected static final String REPLICATION_RATE_BY_BYTES = "replicationRateByBytes";
+
+ /**
+ * Get the rate of rereplication. Default is -1, that not throttle.
+ *
+ * @return rate of rereplication (read bytes per second)
+ */
+ public int getReplicationRateByBytes() {
+ return getInt(REPLICATION_RATE_BY_BYTES, -1);
+ }
+
+ /**
+ * Set the rate of rereplication writes.
+ *
+ * @param rate rate of rereplication writes (writes bytes per second)
+ *
+ * @return ServerConfiguration
Review comment:
it does return ClientConfiguration
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
##########
@@ -200,6 +200,29 @@
protected static final String CLIENT_CONNECT_BOOKIE_UNAVAILABLE_LOG_THROTTLING =
"clientConnectBookieUnavailableLogThrottling";
+ protected static final String REPLICATION_RATE_BY_BYTES = "replicationRateByBytes";
+
+ /**
+ * Get the rate of rereplication. Default is -1, that not throttle.
Review comment:
```suggestion
* Get the bytes rate of re-replication. Default value is -1 which it means entries will replicated without any throttling activity.
```
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java
##########
@@ -200,6 +200,29 @@
protected static final String CLIENT_CONNECT_BOOKIE_UNAVAILABLE_LOG_THROTTLING =
"clientConnectBookieUnavailableLogThrottling";
+ protected static final String REPLICATION_RATE_BY_BYTES = "replicationRateByBytes";
+
+ /**
+ * Get the rate of rereplication. Default is -1, that not throttle.
+ *
+ * @return rate of rereplication (read bytes per second)
Review comment:
writes bytes per second ?
##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java
##########
@@ -112,10 +119,16 @@ public LedgerFragmentReplicator(BookKeeper bkc, StatsLogger statsLogger) {
numBytesWritten = this.statsLogger.getOpStatsLogger(NUM_BYTES_WRITTEN);
readDataLatency = this.statsLogger.getOpStatsLogger(READ_DATA_LATENCY);
writeDataLatency = this.statsLogger.getOpStatsLogger(WRITE_DATA_LATENCY);
+ if (conf.getReplicationRateByBytes() > 0) {
+ this.replicationThrottle = new Throttler(conf.getReplicationRateByBytes());
+ }
+ totalReplicatedEntriesSize = 0;
Review comment:
I'd suggest to keep this 3 fields into the Throttler object to be more self-contained
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org