virajjasani commented on a change in pull request #2261:
URL: https://github.com/apache/hbase/pull/2261#discussion_r476354758
##########
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:
Same as above:
Using common API for proto seems almost impossible. One use-case of slowLog
is being handled by RSRpc services whereas the other for balancerDecision is
being managed by MasterRpc services. Both have different sets of Rpc
implementations written and handled at different places.
Moreover, at RPC level, we can't achieve abstraction since protobuf doesn't
support `is-a` relationship among proto `messages`.
##########
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<OnlineLogRecord> getSlowLogResponses(final Set<ServerName> serverNames,
- final LogQueryFilter logQueryFilter) throws IOException;
+ default List<OnlineLogRecord> getSlowLogResponses(final Set<ServerName>
serverNames,
Review comment:
This is deprecated API and by IA definitions, we can't change the
signature of this without breaking minor release compatibility? That's why we
made this default implementation to use new API internally.
##########
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:
Using common API for proto seems almost impossible. One use-case of
slowLog is being handled by RSRpc services whereas the other for
balancerDecision is being managed by MasterRpc services. Both have different
sets of Rpc implementations written and handled at different places.
Moreover, at RPC level, we can't achieve abstraction since protobuf doesn't
support `is-a` relationship among proto `messages`.
##########
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/RawAsyncHBaseAdmin.java
##########
@@ -4212,4 +4213,29 @@ private void getProcedureResult(long procId,
CompletableFuture<Void> future, int
(s, c, req, done) -> s.updateRSGroupConfig(c, req, done), resp
-> null))
).call();
}
+
+ private CompletableFuture<List<LogEntry>> getBalancerDecisions(
Review comment:
This is private utility method being used by generic API on L4230:
```
public CompletableFuture<List<LogEntry>> getLogEntries(LogRequest
logRequest, int limit) {
```
Anything specific I can mention in method name to improve readability and
the fact that this is utility method?
##########
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<List<OnlineLogRecord>> getSlowLogResponses(final
Set<ServerName> 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<List<OnlineLogRecord>> getSlowLogResponses(
Review comment:
This is deprecated API and by IA definitions, we can't change the
signature of this without breaking minor release compatibility? That's why we
made this default implementation to use new API internally.
##########
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();
}
+
+ @Override
+ public MasterProtos.BalancerDecisionsResponse
getBalancerDecisions(RpcController controller,
Review comment:
At RPC level, we can't achieve abstraction since protobuf doesn't
support `is-a` relationship among proto `messages`.
##########
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 filter payload might be used to filter more as you said, you are
right. But let me update javadoc to remove `limit` from description. Also, we
don't need to remove mention of slow log because this filter is slowLog
specific. Parent request payload, which is generic is `LogRequest` (which is
empty so far but I have kept it IS.Evolving just in case we can identify some
common filter param applicable to all use-cases).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
