Apache9 commented on a change in pull request #3690:
URL: https://github.com/apache/hbase/pull/3690#discussion_r720648092
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##########
@@ -3822,6 +3824,26 @@ private void getProcedureResult(long procId,
CompletableFuture<Void> future, int
return future;
}
+ @Override
+ public CompletableFuture<Map<String, Long>>
updateCompactionServerTotalThroughput(long upperBound,
+ long lowerBound, long offPeak) {
+ CompletableFuture<Map<String, Long>> future = this.<Map<String, Long>>
newMasterCaller().action(
+ (controller, stub) -> this.<UpdateCompactionServerTotalThroughputRequest,
+ UpdateCompactionServerTotalThroughputResponse, Map<String, Long>> call(
+ controller, stub,
UpdateCompactionServerTotalThroughputRequest.newBuilder()
+
.setMaxThroughputUpperBound(upperBound).setMaxThroughputLowerBound(lowerBound)
+ .setMaxThroughputOffPeak(offPeak).build(),
+ (s, c, req, done) -> s.updateCompactionServerTotalThroughput(c, req,
done), resp -> {
+ Map<String, Long> result = new HashMap<>();
+ result.put("UpperBound", resp.getMaxThroughputUpperBound());
Review comment:
Why return the result like this? The comment says "the now total
throughput of all compaction servers", so I thought the key of the map is the
name of a compaction server...
If we just want to return 3 long values, why not introduce a new class?
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/MiniHBaseCluster.java
##########
@@ -107,6 +107,15 @@ public MiniHBaseCluster(Configuration conf, int
numMasters, int numAlwaysStandBy
regionserverClass);
}
+ public MiniHBaseCluster(Configuration conf, int numMasters, int
numAlwaysStandByMasters,
Review comment:
I think all the test changes could be done as a separated issue.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/LocalHBaseCluster.java
##########
@@ -74,7 +74,7 @@
private final Configuration conf;
private final Class<? extends HMaster> masterClass;
private final Class<? extends HRegionServer> regionServerClass;
-
+ private final Class<? extends HCompactionServer> compactionServerClass;
Review comment:
This could be a separated issue to make this PR smaller?
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/throttle/PressureAwareThroughputController.java
##########
@@ -64,9 +64,9 @@
}
}
- protected long maxThroughputUpperBound;
+ protected volatile long maxThroughputUpperBound;
Review comment:
I think the old design is to recreate the controller so we do not need
volatile here? Why change the behavior?
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
##########
@@ -1459,6 +1459,16 @@
*/
CompletableFuture<Boolean> switchCompactionOffload(boolean enable);
+ /**
+ * update compaction server total throughput bound
+ * @param upperBound the total throughput upper bound of all compaction
servers
+ * @param lowerBound the total throughput lower bound of all compaction
servers
+ * @param offPeak the total throughput offPeak bound of all compaction
servers
Review comment:
offPeakBound?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
