apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r473435176
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##########
@@ -4006,9 +4007,9 @@ private void getProcedureResult(long procId,
CompletableFuture<Void> future, int
}
}
- private CompletableFuture<List<OnlineLogRecord>>
getSlowLogResponseFromServer(
+ private CompletableFuture<List<LogEntry>> getSlowLogResponseFromServer(
Review comment:
Comment on admin interface has implications here. (_This needs an
optional parameter to allow constrained clients to limit the size of the
returned list of LogEntry._)
##########
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 {
+ optional uint32 limit = 5 [default = 250];
Review comment:
This default seems low, and should be an interface method param option
and a class file constant, not a proto constant, IMHO.
The limit should be generic to the getList() interface and not specific to
the log entry type.
The default should align with the server side default ring buffer size. The
idea is the client will get all of the entries by default, but can ask for
fewer depending on use case or constraints.
##########
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:
Comment on admin interface has implications here. (_This needs an
optional parameter to allow constrained clients to limit the size of the
returned list of LogEntry._)
##########
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
+ */
[email protected]
+public class BalancerDecisionRequest extends LogRequest {
+
+ private int limit = 250;
+
+ public int getLimit() {
Review comment:
Huh, limit supported here but not in admin API. What am I missing?
##########
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:
For this and other variables controlling whether or not we record
decisions or events, some variant of "record" could be in the variable name to
make this plain. This is "record" as verb, not as noun. Consider
isBalancerDecisionRecording or isBalancerDecisionRecordingEnabled. We don't
need to call the elements FooBarRecords, but when deciding to put them in a
ring buffer or not we could be said to be "recording" FooBar entries or not.
Helps with clarity I think but just mentioned for your consideration.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/RpcLogDetails.java
##########
@@ -40,7 +40,7 @@
public RpcLogDetails(RpcCall rpcCall, Message param, String clientAddress,
long responseSize,
String className, boolean isSlowLog, boolean isLargeLog) {
- super(NamedQueueEvent.SLOW_LOG);
+ super(0);
Review comment:
The idea of using ordinals is we can encode them into message types and
be more gentle at runtime if missing one (as opposed to what Java would do by
default, a linkage error I think). However for code I think enums provide
documentation of intent and magic constants should be avoided. Or, at least,
use a static integer constant named "SLOW_LOG"...
##########
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableGetMultiThreaded.java
##########
@@ -211,6 +210,11 @@ public String explainFailure() throws Exception {
LOG.info("====== Move meta done ======");
Thread.sleep(5000);
}
+ BalancerDecisionRequest balancerDecisionRequest = new
BalancerDecisionRequest();
+ balancerDecisionRequest.setLimit(2);
Review comment:
Per other comments limit should be lifted into admin API not set per
request type.
##########
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:
This needs an optional parameter to allow constrained clients to limit
the size of the returned list of LogEntry.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -2339,9 +2342,16 @@ boolean snapshotCleanupSwitch(final boolean on, final
boolean synchronous)
* @param logQueryFilter filter to be used if provided (determines slow /
large RPC logs)
* @return online slowlog response list
* @throws IOException if a remote or network exception occurs
+ * @deprecated since 2.4.0 and will be removed in 4.0.0.
Review comment:
As I mentioned in my earlier comments, I recognize it's unfortunate to
have an almost immediate deprecation of this but it's better to move to a
generic API now then bake in more special case APIs per ring buffer element
type.
So +1 to this.
##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##########
@@ -64,6 +66,7 @@
import org.apache.hadoop.hbase.snapshot.UnknownSnapshotException;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.Pair;
+import
org.apache.hbase.thirdparty.org.apache.commons.collections4.CollectionUtils;
Review comment:
Is there a nonshaded collections util available? Might be an issue when
backporting. Just a nit.
##########
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:
Comment on admin interface has implications here. (_This needs an
optional parameter to allow constrained clients to limit the size of the
returned list of LogEntry._)
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/NamedQueuePayload.java
##########
@@ -30,16 +30,38 @@
public class NamedQueuePayload {
public enum NamedQueueEvent {
- SLOW_LOG
+ SLOW_LOG(0),
+ BALANCE_DECISION(1);
+
+ private final int value;
+
+ NamedQueueEvent(int i) {
+ this.value = i;
+ }
+
+ public static NamedQueueEvent getEventByOrdinal(int value){
+ switch (value) {
+ case 0: {
+ return SLOW_LOG;
+ }
+ case 1: {
+ return BALANCE_DECISION;
+ }
+ default: {
+ throw new IllegalArgumentException("Failed to retrieve NamedQueue
Event");
Review comment:
"NamedQueue event with ordinal %d not defined"
##########
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:
Just a thought:
Can we leave this blank by default? And then do a runtime scan of classes
implementing the log queue interface if this param is blank, to include them
all by default. Over the long run this would be less prone to error.
##########
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:
Probably should be BalancerDecisionsRequest, but just a nit (usually
there will be more than one decision, right?)
##########
File path: hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
##########
@@ -1605,6 +1605,10 @@
*/
public static final int BATCH_ROWS_THRESHOLD_DEFAULT = 5000;
+ public static final String BALANCER_DECISION_BUFFER_ENABLED =
Review comment:
Should probably be defined in a base load balancer class or interface
instead.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/BalancerDecisionDetails.java
##########
@@ -0,0 +1,49 @@
+/*
+ *
+ * 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.namequeues;
+
+import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.hadoop.hbase.client.BalancerDecision;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Balancer decision details that would be passed on to ring buffer for history
+ */
[email protected]
+public class BalancerDecisionDetails extends NamedQueuePayload {
+
+ private final BalancerDecision balancerDecision;
+
+ public BalancerDecisionDetails(BalancerDecision balancerDecision) {
+ super(1);
+ this.balancerDecision = balancerDecision;
+ }
+
+ public BalancerDecision getBalancerDecisionRecords() {
Review comment:
Could just be getBalancerDecisions()
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/BalancerDecisionDetails.java
##########
@@ -0,0 +1,49 @@
+/*
+ *
+ * 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.namequeues;
+
+import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.hadoop.hbase.client.BalancerDecision;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Balancer decision details that would be passed on to ring buffer for history
+ */
[email protected]
+public class BalancerDecisionDetails extends NamedQueuePayload {
+
+ private final BalancerDecision balancerDecision;
+
+ public BalancerDecisionDetails(BalancerDecision balancerDecision) {
+ super(1);
+ this.balancerDecision = balancerDecision;
+ }
+
+ public BalancerDecision getBalancerDecisionRecords() {
+ return balancerDecision;
+ }
+
+ @Override
+ public String toString() {
+ return new ToStringBuilder(this)
+ .append("balancerDecisionRecords", balancerDecision)
Review comment:
Could be just "balancerDecisions"
##########
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:
I hope this won't be too difficult to port back to Java 7 for branch-1.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/RSRpcServices.java
##########
@@ -3966,7 +3966,7 @@ public SlowLogResponses getSlowLogResponses(final
RpcController controller,
}
List<SlowLogPayload> slowLogPayloads;
NamedQueueGetRequest namedQueueGetRequest = new NamedQueueGetRequest();
-
namedQueueGetRequest.setNamedQueueEvent(NamedQueuePayload.NamedQueueEvent.SLOW_LOG);
+ namedQueueGetRequest.setNamedQueueEvent(0);
Review comment:
See related comment.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/impl/SlowLogQueueService.java
##########
@@ -201,7 +201,7 @@ public NamedQueueGetResponse
getNamedQueueRecords(NamedQueueGetRequest request)
slowLogPayloads = getSlowLogPayloads(slowLogResponseRequest);
}
NamedQueueGetResponse response = new NamedQueueGetResponse();
- response.setNamedQueueEvent(NamedQueuePayload.NamedQueueEvent.SLOW_LOG);
+ response.setNamedQueueEvent(0);
Review comment:
See related comment.
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/request/NamedQueueGetRequest.java
##########
@@ -46,20 +48,29 @@ public void setSlowLogResponseRequest(
this.slowLogResponseRequest = slowLogResponseRequest;
}
+ public MasterProtos.BalancerDecisionRequest getBalancerDecisionRequest() {
+ return balancerDecisionRequest;
+ }
+
+ public void setBalancerDecisionRequest(
+ MasterProtos.BalancerDecisionRequest balancerDecisionRequest) {
+ this.balancerDecisionRequest = balancerDecisionRequest;
+ }
+
public NamedQueuePayload.NamedQueueEvent getNamedQueueEvent() {
return namedQueueEvent;
}
- public void setNamedQueueEvent(NamedQueuePayload.NamedQueueEvent
namedQueueEvent) {
- this.namedQueueEvent = namedQueueEvent;
+ public void setNamedQueueEvent(int eventOrdinal) {
+ this.namedQueueEvent =
NamedQueuePayload.NamedQueueEvent.getEventByOrdinal(eventOrdinal);
}
@Override
public String toString() {
return new ToStringBuilder(this)
.append("slowLogResponseRequest", slowLogResponseRequest)
.append("namedQueueEvent", namedQueueEvent)
+ .append("balancerDecisionRequest", balancerDecisionRequest)
Review comment:
"balancerDecisionsRequest"
----------------------------------------------------------------
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:
[email protected]
