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

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

virajjasani commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r482700776



##########
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 is our Admin endpoint:
   ```
   List<LogEntry> getLogEntries(LogRequest logRequest) throws IOException;
   ```
   Now when client uses this to get slowLog response, they can provide `limit` in specific payload which extends `LogRequest` and send it over, similarly they can provide `limit` in BalancerDecisionRequest which extends `LogRequest` and get the response accordingly, both request payloads that extend `LogRequest` are anyways public interfaces for clients. Now, client would rather provide `limit` in request payload than as separate argument on this Admin endpoint (method signature). Does that sound good?

##########
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/BalancerDecisionRequest.java
##########
@@ -0,0 +1,64 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.hbase.client;
+
+import org.apache.commons.lang3.builder.EqualsBuilder;
+import org.apache.commons.lang3.builder.HashCodeBuilder;
+import org.apache.commons.lang3.builder.ToStringBuilder;
+import org.apache.yetus.audience.InterfaceAudience;
+
+/**
+ * Balancer decision request payload with filter attributes
+ */
+@InterfaceAudience.Private
+public class BalancerDecisionRequest extends LogRequest {
+
+  private int limit = 250;
+
+  public int getLimit() {

Review comment:
       This is public non-RPC class. It extends `LogRequest` and public Admin API endpoint is:
   ```
     List<LogEntry> getLogEntries(LogRequest logRequest) throws IOException;
   ```
   And hence, this class goes as request param to this API by extending `LogRequest`, so we anyways have this `limit` on public class for imposing all limits and filters (BalancerDecision so far doesn't have filters though, so just limit).

##########
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 exactly what I am trying to do: 
   1. For SlowLog use-case, this class exposes `limit`:
       `public class LogQueryFilter extends LogRequest`
   2. For BalancerDecision use-case, this class exposes `limit`:
       `public class BalancerDecisionRequest extends LogRequest`




----------------------------------------------------------------
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