[GitHub] [hbase] apurtell commented on a change in pull request #2261: HBASE-24528 : BalancerDecision queue implementation in HMaster with Admin API

[GitBox <gi...@apache.org>]

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<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:
       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<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:
       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<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:
       
   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 +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:
       -1 to limits on non public RPC request classes.

##########
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:
       In the future please do not mark unresolved conversations as resolved. 




----------------------------------------------------------------
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:
users@infra.apache.org