virajjasani commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r473904758
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##########
@@ -1057,4 +1051,9 @@ public void updateRSGroupConfig(String groupName,
Map<String, String> configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List<LogEntry> getLogEntries(LogRequest logRequest) throws
IOException {
Review comment:
Same as above, `limit` is included in request payload by individual
use-cases.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BalancerDecisionRequest.java
##########
@@ -0,0 +1,64 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hbase.client;
+
+import org.apache.commons.lang3.builder.EqualsBuilder;
+import org.apache.commons.lang3.builder.HashCodeBuilder;
+import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Balancer decision request payload with filter attributes
+ */
+@InterfaceAudience.Private
+public class BalancerDecisionRequest extends LogRequest {
+
+ private int limit = 250;
+
+ public int getLimit() {
Review comment:
This is the request payload coming to Admin API. Since this class is
extending `LogRequest`.
Detailed comment on Admin API: `limit` is included in request payload by
individual use-cases.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -222,6 +226,14 @@ public synchronized void setConf(Configuration conf) {
curFunctionCosts= new Double[costFunctions.size()];
tempFunctionCosts= new Double[costFunctions.size()];
+
+ boolean isBalancerDecisionEnabled = getConf()
Review comment:
Definitely, with verb it looks good.
##########
File path: hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto
##########
@@ -693,6 +694,14 @@ message SwitchExceedThrottleQuotaResponse {
required bool previous_exceed_throttle_quota_enabled = 1;
}
+message BalancerDecisionRequest {
Review comment:
Oh I see, this is just request payload we are sending over to RPC
endpoint. But yeah, it could be `Decisions`
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -2472,4 +2482,13 @@ boolean snapshotCleanupSwitch(final boolean on, final
boolean synchronous)
*/
void updateRSGroupConfig(String groupName, Map<String, String>
configuration) throws IOException;
+ /**
+ * Retrieve recent online records from HMaster / RegionServers.
+ * Examples include slow/large RPC logs, balancer decisions by master.
+ *
+ * @param logRequest request payload with possible filters
+ * @return Log entries representing online records from servers
+ * @throws IOException if a remote or network exception occurs
+ */
+ List<LogEntry> getLogEntries(LogRequest logRequest) throws IOException;
Review comment:
We can consider `limit` as also one of the request attribute right?
Hence, we are leaving upto individual use-cases to provide limit and both
slowLog and balancerDecision do have `limit` provided as part of their request
payload and both use-cases consider limit:
(All references are for 1. BalancerDecision followed by 2. SlowLog)
```
1.
public class BalancerDecisionRequest extends LogRequest {
private int limit = 250;
public int getLimit() {
...
...
2.
public class LogQueryFilter extends LogRequest {
...
...
private int limit = 10;
...
...
```
And, here on the server side, we consider limiting records for both
use-cases:
```
1.
public class BalancerDecisionQueueService implements NamedQueueService {
...
...
@Override
public NamedQueueGetResponse getNamedQueueRecords(NamedQueueGetRequest
request) {
...
...
int limit = Math.min(request.getBalancerDecisionRequest().getLimit(),
balancerDecisions.size());
// filter limit if provided
balancerDecisions = balancerDecisions.subList(0, limit);
...
...
2.
public class LogHandlerUtils {
...
...
public static List<TooSlowLog.SlowLogPayload> getFilteredLogs(
AdminProtos.SlowLogResponseRequest request,
List<TooSlowLog.SlowLogPayload> logPayloadList) {
...
...
int limit = Math.min(request.getLimit(), logPayloadList.size());
return logPayloadList.subList(0, limit);
}
```
Also, this is where we convert Admin request payload to Protobuf request
payload before passing on to servers over network call:
```
1.
private CompletableFuture<List<LogEntry>> getBalancerDecisions(
BalancerDecisionRequest balancerDecisionRequest) {
return this.<List<LogEntry>>newMasterCaller()
.action((controller, stub) ->
this.call(controller, stub,
MasterProtos.BalancerDecisionRequest.newBuilder()
/** => **/ .setLimit(balancerDecisionRequest.getLimit()).build(),
MasterService.Interface::getBalancerDecisions,
ProtobufUtil::toBalancerDecisionResponse))
.call();
}
2.
public final class RequestConverter {
...
...
public static SlowLogResponseRequest buildSlowLogResponseRequest(
final LogQueryFilter logQueryFilter) {
...
...
return builder.setLimit(logQueryFilter.getLimit()).build();
}
```
##########
File path: hbase-common/src/main/resources/hbase-default.xml
##########
@@ -1994,7 +1994,7 @@ possible configurations would overwhelm and obscure the
important.
</property>
<property>
<name>hbase.namedqueue.provider.classes</name>
- <value>org.apache.hadoop.hbase.namequeues.impl.SlowLogQueueService</value>
+
<value>org.apache.hadoop.hbase.namequeues.impl.SlowLogQueueService,org.apache.hadoop.hbase.namequeues.impl.BalancerDecisionQueueService</value>
Review comment:
Hmm, well keeping it blank by default is not a bad choice but Java's way
of identifying what all classes are implementing an interface seems bit dirty
to me :( (unless you are aware of better reflection libraries which can nail
this in better way)
In previous PR https://github.com/apache/hbase/pull/2052 , I discussed this
with Wellington also. Initially, I thought yeah, let's find all implementors
run time, should not be a big deal, but I could not find one way simple enough
to not ruin our code, and hence ended up doing this. Everytime we add new
namedQueue implmentation, we can update `hbase-default` and it's
functionalities will be taken care of.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -234,26 +246,21 @@ private void loadCustomCostFunctions(Configuration conf) {
return;
}
- costFunctions.addAll(Arrays.stream(functionsNames)
- .map(c -> {
- Class<? extends CostFunction> klass = null;
- try {
- klass = (Class<? extends CostFunction>) Class.forName(c);
- } catch (ClassNotFoundException e) {
- LOG.warn("Cannot load class " + c + "': " + e.getMessage());
- }
- if (null == klass) {
- return null;
- }
-
- CostFunction reflected = ReflectionUtils.newInstance(klass,
conf);
- LOG.info("Successfully loaded custom CostFunction '" +
- reflected.getClass().getSimpleName() + "'");
-
- return reflected;
- })
- .filter(Objects::nonNull)
- .collect(Collectors.toList()));
+ costFunctions.addAll(Arrays.stream(functionsNames).map(c -> {
Review comment:
That's true, nothing to worry!
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
##########
@@ -1673,4 +1682,13 @@
* @throws IOException if a remote or network exception occurs
*/
CompletableFuture<Void> updateRSGroupConfig(String groupName, Map<String,
String> configuration);
+
+ /**
+ * Retrieve recent online records from HMaster / RegionServers.
+ * Examples include slow/large RPC logs, balancer decisions by master.
+ *
+ * @param logRequest request payload with possible filters
+ * @return Log entries representing online records from servers
+ */
+ CompletableFuture<List<LogEntry>> getLogEntries(LogRequest logRequest);
Review comment:
Same as above, `limit` is included in request payload by individual
use-cases.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
