[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r489023341
##
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 BalancerDecisionsRequest {
+ optional uint32 limit = 1;
+}
+
+message BalancerDecisionsResponse {
Review comment:
Ok.
Please add a comment that explains this in the proto file where these
definitions are made. Can be done at commit time, np.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r487301224
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
##
@@ -1673,4 +1697,20 @@
* @throws IOException if a remote or network exception occurs
*/
CompletableFuture updateRSGroupConfig(String groupName, Map configuration);
+
+ /**
+ * Retrieve recent online records from HMaster / RegionServers.
+ * Examples include slow/large RPC logs, balancer decisions by master.
+ *
+ * @param serverNames servers to retrieve records from, useful in case of
records maintained by
Review comment:
What happens when we have multiple masters? I think just a doc update is
needed here to indicate the log for servertype=MASTER will only come from the
currently active master. Can be done at commit time.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ServerType.java
##
@@ -0,0 +1,33 @@
+/*
+ *
+ * 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.yetus.audience.InterfaceAudience;
+
+/**
+ * Select server type i.e destination for RPC request associated with ring
buffer.
+ * e.g slow/large log records are maintained by HRegionServer, whereas
balancer decisions
+ * are maintained by HMaster.
+ */
+@InterfaceAudience.Public
+public enum ServerType {
+ HMASTER,
Review comment:
We try to not use 'H' prefixes in new code. Please just call these
MASTER and REGION_SERVER. Can be fixed at commit time.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##
@@ -3626,4 +3643,49 @@ public static CheckAndMutate
toCheckAndMutate(ClientProtos.Condition condition,
throw new DoNotRetryIOException(e.getMessage());
}
}
+
+ public static List toBalancerDecisionResponse(
+ HBaseProtos.LogEntry logEntry) {
+try {
+ final String logClassName = logEntry.getLogClassName();
+ Class logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("BalancerDecisionsResponse")) {
Review comment:
Same comment as above.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##
@@ -3503,14 +3508,26 @@ private static OnlineLogRecord getSlowLogRecord(
/**
* Convert AdminProtos#SlowLogResponses to list of {@link OnlineLogRecord}
*
- * @param slowLogResponses slowlog response protobuf instance
+ * @param logEntry slowlog response protobuf instance
* @return list of SlowLog payloads for client usecase
*/
- public static List toSlowLogPayloads(
- final AdminProtos.SlowLogResponses slowLogResponses) {
-List onlineLogRecords =
slowLogResponses.getSlowLogPayloadsList()
-
.stream().map(ProtobufUtil::getSlowLogRecord).collect(Collectors.toList());
-return onlineLogRecords;
+ public static List toSlowLogPayloads(
+ final HBaseProtos.LogEntry logEntry) {
+try {
+ final String logClassName = logEntry.getLogClassName();
+ Class logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("SlowLogResponses")) {
Review comment:
This is fine but leaves it up to the client to wonder if the empty list
is because there were no SlowLogResponses or if what was returned was valid
protobuf but we found something else besides the expected entry type. Maybe
throw an exception if the expected type is not encoded? Would help catch future
changes that break compat.
##
File path: hbase-protocol-shaded/src/main/protobuf/HBase.proto
##
@@ -273,4 +273,14 @@ message RegionLocation {
required RegionInfo region_info = 1;
optional ServerName server_name = 2;
required int64 seq_num = 3;
-}
\ No newline at end of file
+}
+
+message LogRequest {
+ required string log_class_name = 1;
+ required bytes log_initializer_message = 2;
Review comment:
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r487301224
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
##
@@ -1673,4 +1697,20 @@
* @throws IOException if a remote or network exception occurs
*/
CompletableFuture updateRSGroupConfig(String groupName, Map configuration);
+
+ /**
+ * Retrieve recent online records from HMaster / RegionServers.
+ * Examples include slow/large RPC logs, balancer decisions by master.
+ *
+ * @param serverNames servers to retrieve records from, useful in case of
records maintained by
Review comment:
What happens when we have multiple masters? I think just a doc update is
needed here to indicate the log for servertype=MASTER will only come from the
currently active master. Can be done at commit time.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ServerType.java
##
@@ -0,0 +1,33 @@
+/*
+ *
+ * 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.yetus.audience.InterfaceAudience;
+
+/**
+ * Select server type i.e destination for RPC request associated with ring
buffer.
+ * e.g slow/large log records are maintained by HRegionServer, whereas
balancer decisions
+ * are maintained by HMaster.
+ */
+@InterfaceAudience.Public
+public enum ServerType {
+ HMASTER,
Review comment:
We try to not use 'H' prefixes in new code. Please just call these
MASTER and REGION_SERVER. Can be fixed at commit time.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##
@@ -3626,4 +3643,49 @@ public static CheckAndMutate
toCheckAndMutate(ClientProtos.Condition condition,
throw new DoNotRetryIOException(e.getMessage());
}
}
+
+ public static List toBalancerDecisionResponse(
+ HBaseProtos.LogEntry logEntry) {
+try {
+ final String logClassName = logEntry.getLogClassName();
+ Class logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("BalancerDecisionsResponse")) {
Review comment:
Same comment as above.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##
@@ -3503,14 +3508,26 @@ private static OnlineLogRecord getSlowLogRecord(
/**
* Convert AdminProtos#SlowLogResponses to list of {@link OnlineLogRecord}
*
- * @param slowLogResponses slowlog response protobuf instance
+ * @param logEntry slowlog response protobuf instance
* @return list of SlowLog payloads for client usecase
*/
- public static List toSlowLogPayloads(
- final AdminProtos.SlowLogResponses slowLogResponses) {
-List onlineLogRecords =
slowLogResponses.getSlowLogPayloadsList()
-
.stream().map(ProtobufUtil::getSlowLogRecord).collect(Collectors.toList());
-return onlineLogRecords;
+ public static List toSlowLogPayloads(
+ final HBaseProtos.LogEntry logEntry) {
+try {
+ final String logClassName = logEntry.getLogClassName();
+ Class logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("SlowLogResponses")) {
Review comment:
This is fine but leaves it up to the client to wonder if the empty list
is because there were no SlowLogResponses or if what was returned was valid
protobuf but we found something else besides the expected entry type. Maybe
throw an exception if the expected type is not encoded? Would help catch future
changes that break compat.
##
File path: hbase-protocol-shaded/src/main/protobuf/HBase.proto
##
@@ -273,4 +273,14 @@ message RegionLocation {
required RegionInfo region_info = 1;
optional ServerName server_name = 2;
required int64 seq_num = 3;
-}
\ No newline at end of file
+}
+
+message LogRequest {
+ required string log_class_name = 1;
+ required bytes log_initializer_message = 2;
Review comment:
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r487301224
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
##
@@ -1673,4 +1697,20 @@
* @throws IOException if a remote or network exception occurs
*/
CompletableFuture updateRSGroupConfig(String groupName, Map configuration);
+
+ /**
+ * Retrieve recent online records from HMaster / RegionServers.
+ * Examples include slow/large RPC logs, balancer decisions by master.
+ *
+ * @param serverNames servers to retrieve records from, useful in case of
records maintained by
Review comment:
What happens when we have multiple masters? I think just a doc update is
needed here to indicate the log for servertype=MASTER will only come from the
currently active master. Can be done at commit time.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ServerType.java
##
@@ -0,0 +1,33 @@
+/*
+ *
+ * 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.yetus.audience.InterfaceAudience;
+
+/**
+ * Select server type i.e destination for RPC request associated with ring
buffer.
+ * e.g slow/large log records are maintained by HRegionServer, whereas
balancer decisions
+ * are maintained by HMaster.
+ */
+@InterfaceAudience.Public
+public enum ServerType {
+ HMASTER,
Review comment:
We try to not use 'H' prefixes in new code. Please just call these
MASTER and REGION_SERVER. Can be fixed at commit time.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##
@@ -3626,4 +3643,49 @@ public static CheckAndMutate
toCheckAndMutate(ClientProtos.Condition condition,
throw new DoNotRetryIOException(e.getMessage());
}
}
+
+ public static List toBalancerDecisionResponse(
+ HBaseProtos.LogEntry logEntry) {
+try {
+ final String logClassName = logEntry.getLogClassName();
+ Class logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("BalancerDecisionsResponse")) {
Review comment:
Same comment as above.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##
@@ -3503,14 +3508,26 @@ private static OnlineLogRecord getSlowLogRecord(
/**
* Convert AdminProtos#SlowLogResponses to list of {@link OnlineLogRecord}
*
- * @param slowLogResponses slowlog response protobuf instance
+ * @param logEntry slowlog response protobuf instance
* @return list of SlowLog payloads for client usecase
*/
- public static List toSlowLogPayloads(
- final AdminProtos.SlowLogResponses slowLogResponses) {
-List onlineLogRecords =
slowLogResponses.getSlowLogPayloadsList()
-
.stream().map(ProtobufUtil::getSlowLogRecord).collect(Collectors.toList());
-return onlineLogRecords;
+ public static List toSlowLogPayloads(
+ final HBaseProtos.LogEntry logEntry) {
+try {
+ final String logClassName = logEntry.getLogClassName();
+ Class logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("SlowLogResponses")) {
Review comment:
This is fine but leaves it up to the client to wonder if the empty list
is because there were no SlowLogResponses or if what was returned was valid
protobuf but we found something else besides the expected entry type. Maybe
throw an exception if the expected type is not encoded? Would help catch future
changes that break compat.
##
File path: hbase-protocol-shaded/src/main/protobuf/HBase.proto
##
@@ -273,4 +273,14 @@ message RegionLocation {
required RegionInfo region_info = 1;
optional ServerName server_name = 2;
required int64 seq_num = 3;
-}
\ No newline at end of file
+}
+
+message LogRequest {
+ required string log_class_name = 1;
+ required bytes log_initializer_message = 2;
Review comment:
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r487310800
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/NamedQueuePayload.java
##
@@ -30,16 +30,39 @@
public class NamedQueuePayload {
public enum NamedQueueEvent {
-SLOW_LOG
+SLOW_LOG(0),
Review comment:
Just to be clear above is a suggestion, feel free to ignore it. ^^
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r487301224
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
##
@@ -1673,4 +1697,20 @@
* @throws IOException if a remote or network exception occurs
*/
CompletableFuture updateRSGroupConfig(String groupName, Map configuration);
+
+ /**
+ * Retrieve recent online records from HMaster / RegionServers.
+ * Examples include slow/large RPC logs, balancer decisions by master.
+ *
+ * @param serverNames servers to retrieve records from, useful in case of
records maintained by
Review comment:
What happens when we have multiple masters? I think just a doc update is
needed here to indicate the log for servertype=MASTER will only come from the
currently active master. Can be done at commit time.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/ServerType.java
##
@@ -0,0 +1,33 @@
+/*
+ *
+ * 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.yetus.audience.InterfaceAudience;
+
+/**
+ * Select server type i.e destination for RPC request associated with ring
buffer.
+ * e.g slow/large log records are maintained by HRegionServer, whereas
balancer decisions
+ * are maintained by HMaster.
+ */
+@InterfaceAudience.Public
+public enum ServerType {
+ HMASTER,
Review comment:
We try to not use 'H' prefixes in new code. Please just call these
MASTER and REGION_SERVER. Can be fixed at commit time.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##
@@ -3626,4 +3643,49 @@ public static CheckAndMutate
toCheckAndMutate(ClientProtos.Condition condition,
throw new DoNotRetryIOException(e.getMessage());
}
}
+
+ public static List toBalancerDecisionResponse(
+ HBaseProtos.LogEntry logEntry) {
+try {
+ final String logClassName = logEntry.getLogClassName();
+ Class logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("BalancerDecisionsResponse")) {
Review comment:
Same comment as above.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
##
@@ -3503,14 +3508,26 @@ private static OnlineLogRecord getSlowLogRecord(
/**
* Convert AdminProtos#SlowLogResponses to list of {@link OnlineLogRecord}
*
- * @param slowLogResponses slowlog response protobuf instance
+ * @param logEntry slowlog response protobuf instance
* @return list of SlowLog payloads for client usecase
*/
- public static List toSlowLogPayloads(
- final AdminProtos.SlowLogResponses slowLogResponses) {
-List onlineLogRecords =
slowLogResponses.getSlowLogPayloadsList()
-
.stream().map(ProtobufUtil::getSlowLogRecord).collect(Collectors.toList());
-return onlineLogRecords;
+ public static List toSlowLogPayloads(
+ final HBaseProtos.LogEntry logEntry) {
+try {
+ final String logClassName = logEntry.getLogClassName();
+ Class logClass =
Class.forName(logClassName).asSubclass(Message.class);
+ Method method = logClass.getMethod("parseFrom", ByteString.class);
+ if (logClassName.contains("SlowLogResponses")) {
Review comment:
This is fine but leaves it up to the client to wonder if the empty list
is because there were no SlowLogResponses or if what was returned was valid
protobuf but we found something else besides the expected entry type. Maybe
throw an exception if the expected type is not encoded? Would help catch future
changes that break compat.
##
File path: hbase-protocol-shaded/src/main/protobuf/HBase.proto
##
@@ -273,4 +273,14 @@ message RegionLocation {
required RegionInfo region_info = 1;
optional ServerName server_name = 2;
required int64 seq_num = 3;
-}
\ No newline at end of file
+}
+
+message LogRequest {
+ required string log_class_name = 1;
+ required bytes log_initializer_message = 2;
Review comment:
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r482602787
##
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:
-1 on limits in non public RPC classes. The Admin API is the public
interface here. These RPC FooRequest and FooResponse classes are implementation
detail and not the place to be doing this.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r482602133
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/LogQueryFilter.java
##
@@ -22,22 +22,27 @@
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;
import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.hadoop.hbase.ServerName;
import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.yetus.audience.InterfaceStability;
+import java.util.Collections;
+import java.util.Set;
/**
* Slow/Large Log Query Filter with all filter and limit parameters
* Used by Admin API: getSlowLogResponses
*/
-@InterfaceAudience.Private
-public class LogQueryFilter {
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public class LogQueryFilter extends LogRequest {
private String regionName;
private String clientAddress;
private String tableName;
private String userName;
- private int limit = 10;
Review comment:
Limits by way of filter will be fine if you want to do it this way.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261: URL: https://github.com/apache/hbase/pull/2261#discussion_r482601358 ## 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 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 getLogEntries(LogRequest logRequest) throws IOException; Review comment: Please define a limit parameter for the Admin API. I'm open to other suggestions, but by "individual use cases" I believe you mean the RPC message classes, and those are not the user facing API, they are an implementation detail. As an alternative you can do the same thing as you did for the SlowLog API where a filter can be supplied, and one thing the filter interface lets you do is specify a limit. I won't approve this without a limit option of some kind in the public user facing admin API. 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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261: URL: https://github.com/apache/hbase/pull/2261#discussion_r482600399 ## 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 updateRSGroupConfig(String groupName, Map 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> getLogEntries(LogRequest logRequest); Review comment: Please define a limit parameter for the Admin API. I'm open to other suggestions, but by "individual use cases" I believe you mean the RPC message classes, and those are not user facing public API. The user facing API here is the Admin API. This is the place to do this. As an alternative you can do the same thing as you did for the SlowLog API where a filter can be supplied, and one thing the filter interface lets you do is specify a limit. I won't approve this without a limit option of some kind in the public user facing admin API. 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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r482600399
##
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 updateRSGroupConfig(String groupName, Map 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> getLogEntries(LogRequest logRequest);
Review comment:
Please define a limit parameter for the Admin API. I'm open to other
suggestions, but by "individual use cases" I believe you mean the RPC message
classes, and those are not public API.
As an alternative you can do the same thing as you did for the SlowLog API
where a filter can be supplied, and one thing the filter interface lets you do
is specify a limit.
I won't approve this without a limit option of some kind in the public user
facing admin API.
##
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:
-1 on limits in non public RPC classes. The Admin API is the public
interface here. These RPC classes are implementation detail and not the place
to be doing this.
##
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 updateRSGroupConfig(String groupName, Map 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> getLogEntries(LogRequest logRequest);
Review comment:
In the future please do not mark conversations that are not resolved as
resolved.
##
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
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 getLogEntries(LogRequest logRequest) throws IOException;
Review comment:
Please define a limit parameter for the Admin API. I'm open to other
suggestions, but by "individual use cases" I believe you mean the RPC message
classes, and those are not public API.
As an alternative you can do the same thing as you did for the SlowLog API
where a filter can be supplied, and one thing the filter interface lets you do
is specify a limit.
I won't approve this without a limit option of some kind in the public user
facing admin API.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/LogQueryFilter.java
##
@@ -22,22
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r477627879
##
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 BalancerDecisionsRequest {
Review comment:
Not impossible at all.
getRingEntries RPC in RS RPC servcies
getRingEntries RPC in master RPC services
> we can't achieve abstraction since protobuf doesn't support is-a
relationship among proto messages.
You have an outer message type that is a class name and a byte string, and
the protobuf for the inner type is serialized into the byte string while the
class name field of the outer type is set to the type to use for parsing the
inner type.
Otherwise you have made moot any work toward making things generic, there
will be new message types and RPC methods for every ring.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r477627879
##
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 BalancerDecisionsRequest {
Review comment:
Not impossible at all.
getRingEntries RPC in RS RPC servcies
getRingEntries RPC in master RPC services
> we can't achieve abstraction since protobuf doesn't support is-a
relationship among proto messages.
You have an outer message type that is a class name and a byte string, and
the protobuf for the inner type is serialized into the byte string while the
class name is set to the type you need to use to deserialize.
Otherwise you have made moot any work toward making things generic, there
will be new message types and RPC methods for every ring.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r477627879
##
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 BalancerDecisionsRequest {
Review comment:
Not impossible at all.
getRingEntries RPC in RS RPC servcies
getRingEntries RPC in master RPC services
> we can't achieve abstraction since protobuf doesn't support is-a
relationship among proto messages.
You have an outer message type that is a class name and a byte string, and
the inner protobuf is stuffed into the byte string while the class name is set
to the type you need to use to deserialize.
Otherwise you have made moot any work toward making things generic, there
will be new message types and RPC methods for every ring.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r477623990
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
##
@@ -1543,9 +1544,17 @@
* @param serverNames Server names to get slowlog responses from
* @param logQueryFilter filter to be used if provided
* @return Online slowlog response list. The return value wrapped by a
{@link CompletableFuture}
- */
- CompletableFuture> getSlowLogResponses(final
Set serverNames,
- final LogQueryFilter logQueryFilter);
+ * @deprecated since 2.4.0 and will be removed in 4.0.0.
+ * Use {@link #getLogEntries(LogRequest, int)} instead.
+ */
+ default CompletableFuture> getSlowLogResponses(
Review comment:
See above
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/LogQueryFilter.java
##
@@ -22,22 +22,27 @@
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;
import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.hadoop.hbase.ServerName;
import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.yetus.audience.InterfaceStability;
+import java.util.Collections;
+import java.util.Set;
/**
* Slow/Large Log Query Filter with all filter and limit parameters
* Used by Admin API: getSlowLogResponses
*/
-@InterfaceAudience.Private
-public class LogQueryFilter {
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public class LogQueryFilter extends LogRequest {
private String regionName;
private String clientAddress;
private String tableName;
private String userName;
- private int limit = 10;
Review comment:
If we can limit the slow log result set size with this filter that is
slow log specific, then we don't need a limit param on the existing admin API
for slow log. So keep this?
Apologies for the back and forth on this.
##
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##
@@ -2339,9 +2340,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.
+ * Use {@link #getLogEntries(LogRequest, int)} instead.
*/
- List getSlowLogResponses(final Set serverNames,
- final LogQueryFilter logQueryFilter) throws IOException;
+ default List getSlowLogResponses(final Set
serverNames,
Review comment:
Yes, this is what I meant by "add a method and leave this one for
backwards compat". Additional methods on an interface are binary compat per
Java spec and our compat guidelines as long as existing method signatures are
maintained.
If the LogQueryFilter can impose a limit, then that would work too.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##
@@ -4212,4 +4213,29 @@ private void getProcedureResult(long procId,
CompletableFuture future, int
(s, c, req, done) -> s.updateRSGroupConfig(c, req, done), resp
-> null))
).call();
}
+
+ private CompletableFuture> getBalancerDecisions(
Review comment:
MasterProtos.BalancerDecisionsRequest is not generic. This is the issue.
We should be using one common generic RPC API to get slow log and balancer ring
entries, and any other named ring entry type. This uses a new special master
RPC for balancer decision.
##
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 BalancerDecisionsRequest {
Review comment:
Not impossible at all.
getRingEntries RPC in RS RPC servcies
getRingEntries RPC in master RPC services
> we can't achieve abstraction since protobuf doesn't support is-a
relationship among proto messages.
You have an outer message type that is a class name and a byte string, and
the inner protobuf is stuffed into the byte string while the class name is set
to the type you need to use to deserialize.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r475896191
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/LogQueryFilter.java
##
@@ -22,22 +22,27 @@
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;
import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.hadoop.hbase.ServerName;
import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.yetus.audience.InterfaceStability;
+import java.util.Collections;
+import java.util.Set;
/**
* Slow/Large Log Query Filter with all filter and limit parameters
* Used by Admin API: getSlowLogResponses
*/
-@InterfaceAudience.Private
-public class LogQueryFilter {
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public class LogQueryFilter extends LogRequest {
private String regionName;
private String clientAddress;
private String tableName;
private String userName;
- private int limit = 10;
Review comment:
This change doesn't track with the javadoc for this class, which says
"Slow/Large Log Query Filter with all filter and limit parameters". Also I
think the comment should be updated to not mention the slow log too.
Anyway, maybe a filter would want to filter more? Not a big deal, just
wondering.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r475894386
##
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##
@@ -2339,9 +2340,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.
+ * Use {@link #getLogEntries(LogRequest, int)} instead.
*/
- List getSlowLogResponses(final Set serverNames,
- final LogQueryFilter logQueryFilter) throws IOException;
+ default List getSlowLogResponses(final Set
serverNames,
Review comment:
No 'limit' parameter here. Improvement should track the changes we are
making to the generic API? Add a method and leave this one for backwards
compat?
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/LogQueryFilter.java
##
@@ -22,22 +22,27 @@
import org.apache.commons.lang3.builder.EqualsBuilder;
import org.apache.commons.lang3.builder.HashCodeBuilder;
import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.hadoop.hbase.ServerName;
import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.yetus.audience.InterfaceStability;
+import java.util.Collections;
+import java.util.Set;
/**
* Slow/Large Log Query Filter with all filter and limit parameters
* Used by Admin API: getSlowLogResponses
*/
-@InterfaceAudience.Private
-public class LogQueryFilter {
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public class LogQueryFilter extends LogRequest {
private String regionName;
private String clientAddress;
private String tableName;
private String userName;
- private int limit = 10;
Review comment:
This change doesn't track with the javadoc for this class, which says
"Slow/Large Log Query Filter with all filter and limit parameters". Also I
think the comment should be updated to not mention the slow log too.
Anyway, does it make sense to have the limit in the admin API but also here?
Maybe a filter would want to filter more? Not a big deal, just wondering.
##
File path: hbase-protocol-shaded/src/main/protobuf/server/master/Master.proto
##
@@ -1123,6 +1132,9 @@ service MasterService {
rpc UpdateRSGroupConfig(UpdateRSGroupConfigRequest)
returns (UpdateRSGroupConfigResponse);
+
+ rpc GetBalancerDecisions(BalancerDecisionsRequest)
Review comment:
Remove. Use getLogEntries() for this instead.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AsyncAdmin.java
##
@@ -1543,9 +1544,17 @@
* @param serverNames Server names to get slowlog responses from
* @param logQueryFilter filter to be used if provided
* @return Online slowlog response list. The return value wrapped by a
{@link CompletableFuture}
- */
- CompletableFuture> getSlowLogResponses(final
Set serverNames,
- final LogQueryFilter logQueryFilter);
+ * @deprecated since 2.4.0 and will be removed in 4.0.0.
+ * Use {@link #getLogEntries(LogRequest, int)} instead.
+ */
+ default CompletableFuture> getSlowLogResponses(
Review comment:
See above comment
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##
@@ -4212,4 +4213,29 @@ private void getProcedureResult(long procId,
CompletableFuture future, int
(s, c, req, done) -> s.updateRSGroupConfig(c, req, done), resp
-> null))
).call();
}
+
+ private CompletableFuture> getBalancerDecisions(
Review comment:
Use the generic API getLogEntries() for this instead of adding one for
balancer decisions.
##
File path: hbase-common/src/main/resources/hbase-default.xml
##
@@ -1994,7 +1994,7 @@ possible configurations would overwhelm and obscure the
important.
hbase.namedqueue.provider.classes
-org.apache.hadoop.hbase.namequeues.impl.SlowLogQueueService
+
org.apache.hadoop.hbase.namequeues.impl.SlowLogQueueService,org.apache.hadoop.hbase.namequeues.impl.BalancerDecisionQueueService
Review comment:
Ok, sounds 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 BalancerDecisionsRequest {
Review comment:
Remove. Use getLogEntries() instead.
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
##
@@ -3289,4 +3295,24 @@ public UpdateRSGroupConfigResponse
updateRSGroupConfig(RpcController controller,
}
return builder.build();
}
+
+
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
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 future, int
}
}
- private CompletableFuture>
getSlowLogResponseFromServer(
+ private CompletableFuture> 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 configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List 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
+ */
+@InterfaceAudience.Private
+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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471706020
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
Ok, but if you dont' accept the majority of this feedback, which would
be fine, we at least need the limit parameter. And also add one for the slow
log if there isn't one there.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471704886
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
So just to be clear, please take some time to think about this, but it
is not a request for change per se.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471701524
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
I agree, that's the trade off here. Some pain now for flexibility later,
or not much pain and no flexibility now or later.
New APIs for each new type is not a wrong choice per se. A benefit to API
per ring type is pressure from API compat concerns and method proliferation
will put a lot of pressure on new potential use cases for this mechanism, will
set a really high bar. Maybe too high, hence my concerns.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471701524
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
I agree, that's the trade off here. Some pain now for flexibility later,
or not much pain and no flexibility now or later. New APIs for each new type.
Either decision is not wrong or right.
A benefit to API per ring type is pressure from API compat concerns and
method proliferation will put a lot of pressure on new potential use cases for
this mechanism, will set a really high bar. Maybe too high, hence my concerns.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471696078
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/NamedQueuePayload.java
##
@@ -30,7 +30,8 @@
public class NamedQueuePayload {
public enum NamedQueueEvent {
-SLOW_LOG
+SLOW_LOG,
Review comment:
I wish I had caught this earlier.
If we have to use an Enum here, add a constructor that defines an ordinal
for each type so we can maintain compatibility by instantiating by our ordinal,
e.g.
NamedQueueEvent(int ordinal) { ... }
SLOW_LOG(1),
BALANCE_DECISION(2),.
...
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471696078
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/namequeues/NamedQueuePayload.java
##
@@ -30,7 +30,8 @@
public class NamedQueuePayload {
public enum NamedQueueEvent {
-SLOW_LOG
+SLOW_LOG,
Review comment:
I wish I had caught this earlier.
If we have to use an Enum here, add a constructor that defines an ordinal
for each type so we can maintain compatibility by instantiating by our ordinal,
e.g.
NamedQueueEvent(int ordinal) { ... }
SLOW_LOG(1),
BALANCE_DECISION(2),.
...
NamedQueueEvent getByOrdinal(int ordinal)
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471692736
##
File path: hbase-shell/src/main/ruby/hbase/admin.rb
##
@@ -1639,6 +1639,18 @@ def recommission_regionserver(server_name_string,
encoded_region_names)
@admin.recommissionRegionServer(server_name, region_names_in_bytes)
end
+
#--
+# Retrieve latest balancer decisions made by LoadBalancers
+def get_balancer_decisions
+ balancer_decisions_responses = @admin.getBalancerDecisions
+ balancer_decisions_resp_arr = []
+ balancer_decisions_responses.each { |balancer_dec_resp|
+balancer_decisions_resp_arr << balancer_dec_resp.toJsonPrettyPrint
+ }
+ puts 'Retrieved BalancerDecision Responses from HMaster'
+ puts balancer_decisions_resp_arr
Review comment:
Only return the array here, this may be used for programmatic things.
Put the part that prints the result into the command impl which invokes this
admin.rb function.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471690236
##
File path:
hbase-thrift/src/main/java/org/apache/hadoop/hbase/thrift2/client/ThriftAdmin.java
##
@@ -1277,4 +1278,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
throw new NotImplementedException("updateRSGroupConfig not supported in
ThriftAdmin");
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
If we had one admin API for master and regionservers that returned
protobuf encoded entries, then the thrift API could support it. First cut could
just pass through the encoded PB.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471671371
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
Something to consider: Rather than adding new API for every ringbuffer
backed type, since the ringbuffers are named, can we just have one API that
retrieves records from a buffer specified by name?
E.g.
public List\ getLogEntries(String name)
Then, LogEntry is a generic type capable of accepting any protobuf encoding.
Then, we derive new types from LogEntry such as BalancerDecision. Have a static
method in LogEntry for instantiating the subclasses by reflection based on what
type is communicated by the protobuf.
If _LogEntry_ is too generic a name, consider _RingEntry_. (I'm not the best
at naming, maybe someone else has a better idea...)
It is a lot easier to add or remove specialized classes as these things
evolve than add or remove methods from public/stable admin APIs.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471676264
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
If we have this type of API, it should be possible to provide a limit,
e.g.
public List\ getLogEntries(String name, int limit);
so that a client that is memory constrained (or wants to be frugal) doesn't
have to worry about invoking this and maybe getting back a list of 5000 entries
or whatever, by passing in a limit of 100, or 10, or ...
It would be fine to also provide a method that doesn't accept a limit, for
convenience. Prerequisite: The methods are generic enough so we don't add a
pair (or more!) for every type. E.g.
public List\ getLogEntries(String name) { this(name,
Integer.MAX_VALUE); }
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471676264
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
If we have this type of API, it should be possible to provide a limit,
e.g.
public List getLogEntries(String name, int limit);
so that a client that is memory constrained (or wants to be frugal) doesn't
have to worry about invoking this and maybe getting back a list of 5000 entries
or whatever, by passing in a limit of 100, or 10, or ...
It would be fine to also provide a method that doesn't accept a limit, for
convenience. Prerequisite: The methods are generic enough so we don't add a
pair (or more!) for every type. E.g.
public List getLogEntries(String name) { this(name, Integer.MAX_VALUE); }
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471676264
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
If we have this type of API, it should be possible to provide a limit,
e.g.
public List getLogEntries(String name, int limit);
so that a client that is memory constrained (or wants to be frugal) doesn't
have to worry about invoking this and maybe getting back a list of 5000 entries
or whatever, by passing in a limit of 100, or 10, or ...
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471671371
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
Something to consider: Rather than adding new API for every ringbuffer
backed type, since the ringbuffers are named, can we just have one API that
retrieves records from a buffer specified by name?
E.g.
public List getLogEntries(String name)
Then, LogEntry is a generic type capable of accepting any protobuf encoding.
Then, we derive new types from LogEntry such as BalancerDecision. Have a static
method in LogEntry for instantiating the subclasses by reflection based on what
type is communicated by the protobuf.
If _LogEntry_ is too generic a name, consider _RingEntry_. (I'm not the best
at naming, maybe someone else has a better idea...)
It is a lot easier to add or remove specialized classes as these things
evolve than add or remove methods from public/stable admin APIs.
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
[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API
apurtell commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r471672318
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
This would mean you'd have to go back and modify any admin API related
to the slow log, which is fine, and desirable (if you accept the premise of
this feedback)
##
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##
@@ -2458,4 +2458,11 @@ boolean snapshotCleanupSwitch(final boolean on, final
boolean synchronous)
*/
void updateRSGroupConfig(String groupName, Map
configuration) throws IOException;
+ /**
+ * Retrieve recent balancer decision factors with region plans from HMaster
in-memory ringbuffer
+ *
+ * @return list of balancer decision records
+ * @throws IOException if a remote or network exception occurs
+ */
+ List getBalancerDecisions() throws IOException;
Review comment:
Just call this BalancerDecision ? Of course it's a record, or an item,
or an element (choose your term for individual item in a collection) so that
just adds letters for no clearer meaning.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BalancerDecisionRecords.java
##
@@ -0,0 +1,147 @@
+/*
+ *
+ * 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.ToStringBuilder;
+import org.apache.hadoop.hbase.util.GsonUtil;
+import org.apache.hbase.thirdparty.com.google.gson.Gson;
+import org.apache.hbase.thirdparty.com.google.gson.JsonSerializer;
+import org.apache.yetus.audience.InterfaceAudience;
+import java.util.List;
+
+/**
+ * History of balancer decisions taken for region movements.
+ */
+@InterfaceAudience.Private
+final public class BalancerDecisionRecords {
+
+ private final String initialFunctionCosts;
+ private final String finalFunctionCosts;
+ private final double initTotalCost;
+ private final double computedTotalCost;
+ private final long computedSteps;
+ private final List regionPlans;
+
+ // used to convert object to pretty printed format
+ // used by toJsonPrettyPrint()
+ private static final Gson GSON = GsonUtil.createGson()
+.setPrettyPrinting()
+.registerTypeAdapter(OnlineLogRecord.class,
(JsonSerializer)
+ (slowLogPayload, type, jsonSerializationContext) -> {
Review comment:
Since this is being used for more than just the slowlog, this parameter
_slowLogPayload_ should be renamed. (First question that comes to mind is what
does slow log have to do with the balancer). Call it _logPayload_?
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/AdminOverAsyncAdmin.java
##
@@ -1057,4 +1057,9 @@ public void updateRSGroupConfig(String groupName,
Map configurat
throws IOException {
get(admin.updateRSGroupConfig(groupName, configuration));
}
+
+ @Override
+ public List getBalancerDecisions() throws
IOException {
Review comment:
Something to consider: Rather than adding new API for every ringbuffer
backed type, since the ringbuffers are named, can we just have one API that
retrieves records from a buffer specified by name?
E.g.
public List getLogEntries(String name)
Then, LogEntry is a generic type capable of accepting any protobuf encoding.
Then, we derive new types from LogEntry such as BalancerDecision. Have a static
method in RingEntry for instantiating the subclasses based on what type is
communicated by the protobuf.
If _LogEntry_ is too generic a name, consider _RingEntry_. (I'm not the best
at naming, maybe someone else has a better idea...)
It is a lot easier to add or remove specialized classes as these things
evolve than add or remove methods from public/stable admin APIs.
##
File path:
